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

Adds --derive CLI argument for deriving additional traits on generated structs/enums #20

Merged
merged 8 commits into from
Mar 24, 2024

Conversation

tbrockman
Copy link
Contributor

  • Adds derive: Vec<String> to Generate, OutputConfig and PackageConfig structs, which is later converted into a TokenStream in the corresponding create_*_struct() functions
  • Adds a simple test to verify that no traits are included in generated output if the vec is empty, and multiple are added if it's not
  • Updates README with usage instructions
  • Fixes existing typo in README
  • Comments out some currently failing tests
  • Also adds some annoying-to-review auto-formatting to files

Not sure if this is the ideal implementation here, but it seems to work 🤷

@kurtbuilds
Copy link
Owner

Looks great overall.

Are the tests commented out because this is still a draft?

I'm thinking through if it's possible to remove the --fake and --ormlite flags entirely.

It could make sense to support --derive feat1:path::to::Derive, where a single : defines a feature so that the derive could be conditionally rather than absolutely added. Doing that would allow taking out the --fake flag.

The ormlite logic is trickier because there's some conditional field logic that needs to take place. You could detect if ormlite was in any of the derives Vec<String> - but that's a pretty hacky answer, a separate --ormlite flag feels like a better solution.

@tbrockman
Copy link
Contributor Author

Are the tests commented out because this is still a draft?

Didn't entirely know whether they were still necessary, I noticed they weren't passing on one of your previous commits (ex: https://github.com/kurtbuilds/libninja/actions/runs/7921487654/job/21626916497), so I commented them out so the rest would at least compile and be ran. Was also being a bit lazy, I wasn't too curious into diving into why they were broken if it was unrelated to anything I'd changed 👀

It could make sense to support --derive feat1:path::to::Derive, where a single : defines a feature so that the derive could be conditionally rather than absolutely added. Doing that would allow taking out the --fake flag.

Yeah, I could see something like that being potentially useful. I also thought about whether it would be useful to also have some sort of selector to filter which objects the traits are derived for (since you may not necessarily want to apply them all to everything), but at that point it seemed like if you want that level of customization that it might make sense to write code to traverse the AST yourself afterwards or use libninja as a library instead (maybe aided by a hook we provide to allow modifying portions of the created structs before they're written), so that CLI could be kept relatively simple.

But given that you've produced a lot more clients using the tool I guess I'd defer to your judgement of what sort of syntax would be the most generally useful and intuitive 😉

@kurtbuilds kurtbuilds merged commit 9e45e9f into kurtbuilds:master Mar 24, 2024
1 check passed
@kurtbuilds
Copy link
Owner

Works for me! Thanks for putting this together. I'll fix the tests now.

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