-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement optionalExpand feature #31
base: master
Are you sure you want to change the base?
Conversation
Looks like a ton of other changes are included in this PR, making it hard to review. Maybe a rebate is in order? |
Yeah, this one is based off of #28, we'll need to merge that before this one. I did this because rebasing these tests are pretty annoying each time for each feature |
Makes sense |
65e4402
to
881e390
Compare
Oof, now I remember why I never fully implemented this. Lots of complexity for something that may see little use. I'll leave it up to you if you think we should move forward with including it. |
I think it is worth doing but I think it highlights some really nice refactoring we could do to clean future features like this up.
i.e. data MoatData
= MoatStruct { structExpandOptional :: Bool
, structAnnotations :: Annotations
, structData... :: Data...
}
| ... becomes data MoatData
= MoatStruct { structGenerationOptions :: GenerationOptions
, structData... :: Data...
}
| ... I think this would allows us to pull out the TH into a separate function and clean the code up quite a bit. |
I think these three things would make new generative features quite a bit easier. The other option was allowing one to change the generative options at generation time, however I prefer the former for two reasons. I experimented with |
I think I'll leave this one open as a reminder to do this refactoring |
See #26
This is particularly nasty. We don't really have a choice other than passing these parameters in with each of
MoatData
s records.We really need a
Reader
inMoatM
to handle passing around the Options. Additionally, aReader
in the pretty swift module would help as well.