WIP Refactor xml conversion to hash #8471
Conversation
There was a problem hiding this comment.
I think that is better to use a case statement here instead of call send based in the class name.
There was a problem hiding this comment.
Really? Hm. I guess it's not the worst, and there's only 4 of them...
There was a problem hiding this comment.
I don't know, seems weird to me.
case value when Array convert_array(value) when Hash convert_hash(value) else raise endThis reads better to me. It is cleaner. It make clear what are the supported types of value without need to read the whole class.
| I like the idea to have a specialized object to do the conversion. 👍 from my side |
There was a problem hiding this comment.
This comment could 🔥
There was a problem hiding this comment.
yeah it was originally there. I think that maybe the code was changed and not the comment.
| Nice stuff @kytrinyx @steveklabnik 👍 |
There was a problem hiding this comment.
Changing this is the line that broke the test. ha!
| We've updated this pull request to handle all of the feedback. We re-checked that we didn't break anything when making these changes, and rolled back a few of the more aggressive changes we made previously. We're thinking that, since most people don't actually use this code, in the future, it might be a good candidate to extract to a plugin. |
|
|
| Should we put the |
| Since it's probably not going to be used on its own, this is fine. Having it in its own file would be nice if it needed to be autoloaded, but it won't. I'm going to squash and then |
Three basic refactors in this PR: * We extracted the logic into a method object. We now don't define a tone of extraneous methods on Hash, even if they were private. * Extracted blocks of the case statement into methods that do the work. This makes the logic more clear. * Extracted complicated if clauses into their own query methods. They often have two or three terms, this makes it much easier to see what they _do_. We took care not to refactor too much as to not break anything, and put comments where we suspect tests are missing. We think ActiveSupport::XMLMini might be a good candidate to move to a plugin in the future.
WIP Refactor xml conversion to hash
6 participants
Hash.from_xml was very complicated, and so we decided to take a stab at
refactoring it. This is a work in progress that we want to get feedback on.
Basically, instead of defining a huge method on Hash, we made a utility
object for actually doing the conversion. This let us break up the big
method into a bunch of smaller ones, so that the process overall is
easier to understand.
However, some of the names kinda suck. Still working on that. But we
wanted to get some feedback on if this kind of change is generally
considered to be better or not before putting more effort into
breaking it up. We think that a bunch of small methods are easier to understand
than one massive one.
Things to work on:
a Hash that's getting converted.
in a few of the if statements. We've left some comments. Tests pass if
you remove them, but we didn't want to before confirming that they
aren't actually needed.
# :nodoc:where appropriate❤️