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

Amend CLI globbing help text. #3302

Merged
merged 3 commits into from
Aug 22, 2019
Merged

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Aug 20, 2019

Update CLI globbing help, post #3296

(We need to be very clear that CLI globbing matches against the current task pool, not abstract tasks in the workflow)

(This may need updating again if we implement pseudo-spawn-on-demand with automatic insertion of tasks not in the pool).


This is a small change with no associated Issue.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Does not need tests (CLI help text only).
  • Appropriate change log entry included.
  • No documentation update required (is documentation).

@hjoliver hjoliver added this to the cylc-8.0a1 milestone Aug 20, 2019
@hjoliver hjoliver self-assigned this Aug 20, 2019
Copy link
Collaborator

@sadielbartholomew sadielbartholomew 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, apart from one small point, & also see my comment RE the position of the warning from the Cylc-7 twin PR, which also applies here & also in this case to the 'note' for the cycle-point-excluded globbing case (e.g. cylc isnert --help).

cylc/flow/option_parsers.py Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member Author

(I'll reposition the warning message on this branch too).

Copy link
Collaborator

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Good clarifications, I think they will help users to understand the real nature of the commands. There are still a few tweaks need here, ideally.

bin/cylc-insert Outdated Show resolved Hide resolved
cylc/flow/option_parsers.py Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member Author

hjoliver commented Aug 22, 2019

Oops - @sadielbartholomew I hadn't requested re-review of the this branch yet (just the 7.8.x) - sorry I should have explicitly said I was still working on it. I've already addressed some of your comments (but not pushed yet). However, I had missed the "TASk" typo - good spotting! Sorry again 😬

@sadielbartholomew
Copy link
Collaborator

Ah sorry @hjoliver, I guess when you said one was done I assumed the other was too. My apologies!

@hjoliver
Copy link
Member Author

Now ready for review again.

Copy link
Collaborator

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Tested as displaying the new or amended content with a sensible order & formatting for the --help output of various affected commands.

@sadielbartholomew sadielbartholomew self-requested a review August 22, 2019 12:57
@sadielbartholomew
Copy link
Collaborator

Actually hold that for a moment, some of the Travis CI test failures may be legit, so just investigating those.

Copy link
Collaborator

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

I have doubled checked with Matt who has been dealing with flaky tests, & all Travis CI test failures (see chunk 2/4) are flaky tests which are known to him (one did fail locally for me, but we determinedit was not related to the change here). So they are unrelated, & this is good to go. Thanks.

@sadielbartholomew sadielbartholomew merged commit 856dd58 into cylc:master Aug 22, 2019
@hjoliver hjoliver deleted the cli-glob-help branch August 22, 2019 20:22
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