Split VariantBuilder into separate builders depending on type#50
Conversation
|
@Bergmann89 I still appear to require workflow approval for every PR. |
|
I will note that this also simplifies the handling of simpleContent massively. Instead of being self-contained now, it instead defers to a SimpleType sub-builder for the content, which considerably decreases complexity. |
|
You have to get your first PR merged, before github automatically start the checks on new PRs. I didn't had a look to this PR yet, I want to merge the other one before I start review on this one. |
|
@Bergmann89 Any requests you have before you review? |
Bergmann89
left a comment
Bergmann89
left a comment
There was a problem hiding this comment.
Just a few small remarks, but overall it looks really good to me — much cleaner than before. Sometimes it really helps to have someone else look at the code and pull you out of your own head! 😄
| /* any type */ | ||
|
|
||
| /// Initialize the type of a `$builder` to any type `$variant`. | ||
| macro_rules! init_any { |
There was a problem hiding this comment.
These two macros are now duplicated for each builder. Could we move them to a utils/common module or just into builder/mod.rs?
There was a problem hiding this comment.
I'd personally like to get rid of the macros all together. Speaking personally, they confused the hell out of me, and I see no benefit compared to simply using functions with #[inline].
There was a problem hiding this comment.
It was simpler to use a macro for this (especially since it was even more complex in an earlier implementation). We can also replace it with a regular function. So, we either add that function now or move the macros into mod.rs. In my opinion, leaving the code duplicated is not an option.
There was a problem hiding this comment.
I've rewritten it to have functions now. I'm not sure I'm 100% happy with it, but I like it better than the macros.
Bergmann89
changed the title
VariantBuilder into separate builders depending on type
2 participants
This PR took a lot of work but now successfully passes all tests. It depends on #43 .