-
Notifications
You must be signed in to change notification settings - Fork 94
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
cylc help fixes #2653
cylc help fixes #2653
Conversation
Use 1st sentence of JOSS paper as description in `cylc help`. Fix: `cylc help control ...`. Fix: `cylc CATEGORY COMMAND --help` Fix: `cylc trigger` abbreviation example. Mention `cylc COMMAND --help`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First two fixes are addressed, but it seems (here as on 'master') that there is still undesired behaviour for one-letter/number ambiguous abbreviations, e.g.:
[sbarth@eld318 bin]$ cylc sc
"sc" is ambiguous.
These commands match the abbreviation "sc":
scan
scp-transfer
[sbarth@eld318 bin]$ cylc s
cylc-scan cylc-set-verbosity cylc-spawn cylc-submit
cylc-scp-transfer cylc-show cylc-start cylc-suite-state
cylc-search cylc-shutdown cylc-stop
Something has gone terribly wrong if you are here...
Additionally, one-letter/number abbreviations which are unambiguous are not getting recognised as such:
[sbarth@eld318 bin]$ cylc 5
cylc-5to6
Something has gone terribly wrong if you are here...
[sbarth@eld318 bin]$ cylc w
cylc-warranty
Something has gone terribly wrong if you are here...
Should now be good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems much better. The only problem I've found on testing now, is abbreviation doesn't work for the prefix help form with category given: cylc prep pri --help
works, but cylc help prep pri
fails. Happy to leave for another PR though (in which we could consider dropping categories)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feedback addressed suitably. Approval waiting on either fix or delegation to a new PR for latest problem spotted by @hjoliver.
Should now be better. |
(I have consolidated the logic to select abbreviated sub-commands.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another issue has been introduced in the new commit, unfortunately:
[sbarth@eld318 bin]$ cylc a
/home/h06/sbarth/cylc.git/bin/cylc: line 193: /net/home/h06/sbarth/cylc.git/bin/cylc-: No such file or directory
[sbarth@eld318 bin]$ cylc nonsense
/home/h06/sbarth/cylc.git/bin/cylc: line 193: /net/home/h06/sbarth/cylc.git/bin/cylc-: No such file or directory
3rd time lucky? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to have got it.
(I have now added a test battery file to stop us from getting it all wrong again.) |
Good idea! Just having a quick look but the test file should now do all the hard work :) |
help_util "$@" | ||
exit 0 | ||
:;; | ||
version|--version|-V|-v) | ||
version|--version|-V) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has the -v
option been removed (so it becomes a 'unknown utility'); I assume we want to stick with either the lower or upper case alias & lower-case v
is commonly used for verbosity specification? If there is agreement about this I am also happy, but we will need to change line 177 of cylc-help
i.e. the relevant help line accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise everything is fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
tests/cli/01-help.t
Outdated
Type "cylc help all" for a list of utilities. | ||
__STDERR__ | ||
run_fail "${TEST_NAME_BASE}-license-aardvark" cylc license aardvark | ||
cmp_ok "${TEST_NAME_BASE}-license-aardvark.stderr" <<'__STDERR__' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not essential, but could consolidate this comparison with the identical expected output from the above command - indeed this would be better as it would ensure they produce the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good now.
Use 1st sentence of JOSS paper as description in
cylc help
.Fix:
cylc help control ...
.Fix:
cylc CATEGORY COMMAND --help
Fix:
cylc trigger
abbreviation example.Mention
cylc COMMAND --help
.Fix #2650.