Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chiroptical
Copy link
Collaborator

@chiroptical chiroptical commented Sep 24, 2021

See #26

This is particularly nasty. We don't really have a choice other than passing these parameters in with each of MoatDatas records.

We really need a Reader in MoatM to handle passing around the Options. Additionally, a Reader in the pretty swift module would help as well.

@chessai
Copy link
Collaborator

chessai commented Sep 24, 2021

Looks like a ton of other changes are included in this PR, making it hard to review. Maybe a rebate is in order?

@chiroptical
Copy link
Collaborator Author

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

@chessai
Copy link
Collaborator

chessai commented Sep 24, 2021

Makes sense

@chessai
Copy link
Collaborator

chessai commented Sep 24, 2021

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.

@chiroptical
Copy link
Collaborator Author

chiroptical commented Sep 25, 2021

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.

  • MoatM needs Reader Options
  • Each record in MoatData should carry a GenerationOptions field to contain things like Annotations, optionalExpand, etc

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.

@chiroptical
Copy link
Collaborator Author

chiroptical commented Sep 25, 2021

  • With a common type for all fields, we can also add a Reader GenerativeOptions to the pretty code too

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 MoatType.Optional carrying this feature but the TH becomes confusing to me quickly. It also means we need to touch more TH each time we add a generative feature (and not easy TH). Second, the decision is made at compile time and the generative options can be chosen for each of your types. e.g. Maybe you only want to optionalExpand for one specific type for some reason.

@chiroptical
Copy link
Collaborator Author

I think I'll leave this one open as a reminder to do this refactoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants