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

Add -C flag to build command #734

Merged
merged 5 commits into from
Aug 26, 2024
Merged

Add -C flag to build command #734

merged 5 commits into from
Aug 26, 2024

Conversation

jeffcharles
Copy link
Collaborator

Description of the change

I want to get early feedback on adding support for the -C flag documented in #702 so I can iterate on the draft as we figure out what makes sense to change.

There's a few changes:

  • CompileAndBuildCommandOpts -> CompileCommandOpts and BuildCommandOpts including splitting out the handling of the compile and build` subcommands.
  • BuildCommandOpts introduces a few extra types to try and get the repeated -C flags to play nicely with clap. There's a lot of copying triple slash comments around, none of it is picked up by clap so not sure if there's something better we can do. I looked at implementing ValueEnum as well but it looks like that's intended only for enums with entirely unit variants. There's also a manual step to translate the Vec<CodegenOption> to CodegenOptionGroup since clap out-of-the-box only allows specifying the -C flag multiple times if the type associated with the option is a Vec.
  • Changes to javy-runner to default to using the build command and support the arguments build supports unless a method has been invoked on the builder to use compile instead. I'm open to a different API.
  • Adding run_with_compile_and_build to most tests to check that the build and compile commands function correctly. This could make more sense as a macro, I thought I'd start with a function because IMO it's easier to follow what the code is doing.

Why am I making this change?

#702

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-core do not require updating CHANGELOG files.
  • I've updated the relevant crate versions if necessary. Versioning policy for library crates
  • I've updated documentation including crate documentation if necessary.

Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction looks good to me! Left some thoughts and minor comments.

crates/cli/src/main.rs Show resolved Hide resolved
crates/cli/src/commands.rs Outdated Show resolved Hide resolved
crates/cli/src/commands.rs Show resolved Hide resolved
crates/runner/src/lib.rs Outdated Show resolved Hide resolved
NoSourceCompression(bool),
}

impl FromStr for CodegenOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had briefly discussed using https://docs.rs/clap/latest/clap/builder/trait.ValueParserFactory.html -- although the current approach seems to work well enough for the current PR. We can always iterate and refactor later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should've mentioned in the description, I looked briefly at using it. My understanding is it works well if you need to keep track additional state outside of parsing the string to whatever type you're parsing it too. But the tradeoff is it forces you to define an additional type for the parser for the value, which is one more type to define. Ultimately since we have to collect a vector of something, I figured it would be easier to just write the state we need to collect to that something. That said, if we want to ensure there are no duplicate keys present and error out in the option collection phase, then having a custom parser would make that possible. Right now if an option is specified more than once, it'll take the last value as the one to use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, if we want to ensure there are no duplicate keys present and error out in the option collection phase, then having a custom parser would make that possible

In the long run, I think we'd want to ensure this.

A custom parser also gives you more control over how to define/manage your group options and group help (e.g., you can have the help be automatically generated from the options without having to manually update the help string manually every time, which might be error prone).

I think it's fine to leave it as it for this first iteration, we can always refactor later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A custom parser also gives you more control over how to define/manage your group options and group help (e.g., you can have the help be automatically generated from the options without having to manually update the help string manually every time, which might be error prone).

Do you mean defining an implementation for possible_values?

@jeffcharles jeffcharles marked this pull request as ready for review August 26, 2024 14:51
@jeffcharles jeffcharles merged commit 9641533 into main Aug 26, 2024
7 checks passed
@jeffcharles jeffcharles deleted the jc.codegen-options branch August 26, 2024 14:51
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