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

Switch arg parsing to clap-derive #110

Merged
merged 2 commits into from
Jan 25, 2022

Conversation

tranzystorekk
Copy link
Contributor

Switches arg parsing from vanilla clap to clap-derive, which reduces boilerplate code (e.g. manual parsing of arg strings to useful types like port numbers, automatic handling of default values)

Major changes:

  • The site dir becomes a positional argument for all subcommands, instead of a global flag, so the usage becomes e.g. firn new my_dir
  • Clap automatically parses arguments to their target types, rejecting invalid values with an error message (i.e. port number)
  • The default for site dir is now ., instead of explicitly calling env::current_dir(). This shifts the interpretation of cwd to the shell and filesystem and should be fine in general, but let me know if that's not okay

Minor changes:

  • The verbosity level is now u8, for logical coherence
  • The "about" strings have been made more uniform and concise

@tranzystorekk
Copy link
Contributor Author

The version, about and author parts of help output can also be automated to the values from the Cargo.toml, if it is desired, but I haven't enabled it yet

@teesloane
Copy link
Owner

I'm open to this change, it's nice. Some stuff is still broken, however:

  • the ./ prefix from #[clap(default_value = ".")] breaks the paths when running firn new | build | serve

If you can get those working (and please make sure to test locally to make sure they do work, since there are no tests yet), then I'll be happy to merge this.

@tranzystorekk
Copy link
Contributor Author

tranzystorekk commented Jan 24, 2022

Seems the glob crate can't handle the . in its patterns, I think the easiest solution is to revert to env::current_dir() after all 😅

EDIT: that also means that running sth like firn serve . never worked, would probably require canonicalizing the path.

@teesloane
Copy link
Owner

Nice - looks good to me. Thanks for contributing.

@teesloane teesloane merged commit d351615 into teesloane:master Jan 25, 2022
@tranzystorekk tranzystorekk deleted the clap-derive branch January 25, 2022 08:24
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