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

Transformed the help management to be implemented not using a command (CRAFT-542). #550

Merged

Conversation

facundobatista
Copy link
Contributor

This has several benefits:

  • The help handling is now more straightforward

  • The COMMAND_GROUPS constant does not include anything related to help anymore (more app isolation ot a future lib)

  • We're able to provide better messages when help was "requested wrong" (e.g. with too many parameters or an incorrect command); IMO this is very important, as we're able to guide the user better in a situation when they're a little lost (there is a reason they are asking for help...)

  • All help handling happens on the "initial args pre-parsing", and not one part there and one part when the command is run (this is transparent for the user, but useful for when Dispatcher will be in other lib)

We have only one minor drawbacks: 'help' itself, as a command, is not listed in the "basic commands group" anymore.

Note I added a ProvideHelpException exception to interrupt the normal flow of execution and finish the process providing a guiding text when help was requested correctly. This was a simple way to achieve this branch targets without moving a lot of code around, it may change in the future to be more simple when we isolate the Dispatcher further.

Copy link
Collaborator

@sergiusens sergiusens left a comment

Choose a reason for hiding this comment

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

Looks good, I do find it a bit confusing as the variable names don't follow the convention for commands, options and arguments seen in other CLI implementations and their conventions. A varname refactor could perhaps help.

charmcraft/main.py Outdated Show resolved Hide resolved
tests/test_help.py Show resolved Hide resolved
@facundobatista facundobatista merged commit 3740fc2 into canonical:master Sep 23, 2021
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.

3 participants