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

fix(cli)!: Show all global options within help for every command #1285

Merged
merged 2 commits into from
Jun 1, 2022

Conversation

phated
Copy link
Member

@phated phated commented May 29, 2022

Closes #1080
Closes #1147 (I think?)

This PR reworks our commander stuff in the CLI so we show all global options for the root/default command and all subcommands. It also allows subcommands to specify specific options they need, which you'll notice with the different -o options throughout the CLI (some that require only files and some that allow directories or files).

This is a pretty hefty PR, so I hope I didn't miss anything. Please give it a thorough review and test combinations of flags with various commands. I tested with --release and --no-link during development.

This PR is marked as breaking because the changes caused the tests to fail, which made me realize we were generating this command grain -S /path/to/stdlib -I /path/to/test/includes run test-file.gr. We no longer allow options before the subcommands because otherwise our CLI thinks you are running the root command and that run is supposed to be a file.

Here are what some of the --help usages output:
Screen Shot 2022-05-29 at 4 38 39 PM

Screen Shot 2022-05-29 at 4 38 27 PM

@phated phated requested a review from a team May 29, 2022 23:39
@marcusroberts
Copy link
Member

I like this approach!

run(wasmFile, program.opts());
});
.addOption(new commander.Option("-p, --print-output").hideHelp())
.action(run);

program
.command("lsp <file>")
Copy link
Member

Choose a reason for hiding this comment

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

Will need to remember to update this for the new LSP

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've been doing the merge and resolves

@phated phated merged commit 1357e16 into main Jun 1, 2022
@phated phated deleted the phated/commands-all-options branch June 1, 2022 20:09
@github-actions github-actions bot mentioned this pull request Jun 1, 2022
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.

CLI - create a common set of parameters that are passed through to commands
4 participants