Refactor(cli): Review of initial version #26
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Let's try to use the convention that an "argument" is anything passed to the program on the command-line that doesn't start with `-`. This name might better reflect that this module handles the command-line parsing, rather than just exporting all the allowed command-line arguments/options.
Otherwise a user might ask: - "Is there a difference between 'Show' and 'Display'?" - "Is the `-h` message different from this message?" - "Does `-v` show the version information of an exercise?" (This could be especially confusing since exercise version strings no longer exist).
Prefer to use the object construction syntax. It's the idiomatic way to construct objects; one advantage is that it still works if the object is changed to a `ref object` (as `new` is called implicitly).
This increases the strictness of the parsing. For example, the command `canonical_data_syncer --help --check` Now prints the help message. Previously, the program silently ignored `--help` and performed `--check`, which is is unusual behaviour for a CLI tool. Fixes: exercism#34
It's arguably simpler to handle `--help` and `--version` internally, rather than setting the `result.action` and returning immediately. And it could be cleaner to reserve `Action` for the program's real functionality. Note that a later commit will add an `enum` of the valid command-line parameters.
Reasons: - `-m, --mode <mode>` is more descriptive than `-d, --default <mode>`, and fits better with `-o, --verbosity <verbosity>`. - We call it `Mode` and `parseMode` internally.
This further tightens up the parsing, and makes the CLI more user-friendly. For example, the command: `canonical_data_syncer --checkk` Now produces: Error: invalid option: '--checkk' And shows the help message. Fixes: exercism#35
The previous commit means that if the user wrote: `canonical_data_syncer help` The program printed: Error: invalid argument: 'help' and then showed the help message. This commit hides that error, so that only the help message is shown for this special case. See: exercism#35
For example, the command: `canonical_data_syncer --verbosity=quiett` Now produces: Error: invalid value for '--verbosity': 'quiett' And shows the help message. Previously, the program silently ignored the invalid value, and operated with the default value. Fixes: exercism#36
For example, the command: `canonical_data_syncer --verbosity` Now produces: Error: '--verbosity' was given without a value And shows the help message. Previously, the program silently ignored the missing value. Fixes: exercism#37
`Arguments` is the configuration of the program, but it doesn't contain what the user physically passed on the command-line. So let's call it `Conf`. The we can use the convention that "argument" refers to everything passed on the command-line that does not start with `-`.
Nim's `parseopt` module defines an "argument" to be everything passed to the program on the command-line that doesn't start with `-`. Therefore it was strange to see e.g. `ExerciseArgument` within the `case` statement `of cmdLongOption, cmdShortOption` rather than `cmdArgument`. It is better to rename each old "argument" as an "option", showing that they start with `-`, and that some of them can take a value. This commit defines the options as an `enum` with an enum-indexed array of the keys, instead of a bunch of consts. We also resolve the issue that `nimpretty` didn't like the old alignment. See: https://nim-lang.org/docs/nep1.html#introduction-spacing-and-whitespace-conventions Note that the name `Opt` avoids a clash with `Option` from the `options` module.
The help message made it seem that the syntax for specifying an option with a value is like: `--verbosity quiet` `--verbosity q` `-o quiet` `-o q` But the only supported syntax was actually: `--verbosity=quiet` `--verbosity=q` `-o=quiet` `-o=q` (Or a colon instead of an equals sign). With this commit, both of the above syntaxes work. Fixes: #30
ErikSchierboom
approved these changes
Oct 13, 2020
| -c, --check Check if there missing tests. Doesn't update the tests. Terminates with a non-zero exit code if any one or more test cases are missing. | ||
| -d, --default <mode> What to do with missing test cases. Allowed values: c[hoose], i[nclude], e[xclude] | ||
| -c, --check Check if there are missing tests. Doesn't update the tests. Terminates with a non-zero exit code if one or more tests are missing | ||
| -m, --mode <mode> What to do with missing test cases. Allowed values: c[hoose], i[nclude], e[xclude] |
There was a problem hiding this comment.
I've gone back and forth on this. Maybe we should add something that explains that setting this means things are handled automatically, whereas not setting it requires interaction.
| @@ -46,6 +46,18 @@ proc showVersion = | |||
| echo &"Canonical Data Syncer v{NimblePkgVersion}" | |||
| quit(0) | |||
|
|
|||
| proc showError(s: string) = | |||
There was a problem hiding this comment.
I don't know if this idiomatic, but I would normally name the parameter error or errorMessage or something like that.
ee7
referenced
this pull request
in ee7/exercism-configlet
Jan 21, 2021
Refactor(cli): Review of initial version
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a review of the initial CLI implementation. The main things are:
--helpmessage showed incorrect syntax.The commits are atomic - please review individual commits and see the commit messages.
And when we have this PR in an acceptable state, please merge without squashing so that we preserve the commit message granularity, and so that
gitcan track the file rename.Fixes: #30
Fixes: #34
Fixes: #35
Fixes: #36
Fixes: #37