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

Added -z option #5605

Merged
merged 2 commits into from
Jun 29, 2023
Merged

Added -z option #5605

merged 2 commits into from
Jun 29, 2023

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jun 28, 2023

Closes #5433
Allow -z simple template variable string lists.

As part of this work I have replaced all instances of load_template_vars (which takes 3 keyword args) with get_template_vars (which takes a CLI options object and pulls the args for load_template_vars out). I have left the separation of the two functions because it reduces testing.

Additionally I have added error handling for this case:

cylc vip -s "FOO" - which previously failed with traceback. It now raises a CylcError subclass which is handled nicely.

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).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Added -z option cylc-doc#618
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

LGTM

cylc/flow/templatevars.py Outdated Show resolved Hide resolved
cylc/flow/templatevars.py Outdated Show resolved Hide resolved
cylc/flow/templatevars.py Outdated Show resolved Hide resolved
cylc/flow/templatevars.py Outdated Show resolved Hide resolved
tests/unit/test_loggingutil.py Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member

(Corrected linked Issue in description from 5543 to #5433)

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

👍 a few trivial changes suggested

cylc/flow/option_parsers.py Outdated Show resolved Hide resolved
cylc/flow/templatevars.py Outdated Show resolved Hide resolved
cylc/flow/templatevars.py Outdated Show resolved Hide resolved
cylc/flow/templatevars.py Outdated Show resolved Hide resolved
cylc/flow/templatevars.py Outdated Show resolved Hide resolved
cylc/flow/templatevars.py Outdated Show resolved Hide resolved
@wxtim wxtim force-pushed the feature.plus_s_list_of_strings branch 2 times, most recently from 876964d to 6362c4a Compare June 29, 2023 08:16
@wxtim wxtim self-assigned this Jun 29, 2023
@wxtim wxtim added this to the cylc-8.2.0 milestone Jun 29, 2023
@wxtim wxtim mentioned this pull request Jun 29, 2023
1 task
CHANGES.md Outdated Show resolved Hide resolved
@wxtim wxtim force-pushed the feature.plus_s_list_of_strings branch from 9242324 to c2343c3 Compare June 29, 2023 10:47
@wxtim wxtim changed the base branch from master to 6.11.x June 29, 2023 10:49
@wxtim wxtim changed the base branch from 6.11.x to master June 29, 2023 10:50
@wxtim wxtim force-pushed the feature.plus_s_list_of_strings branch from 9a4b706 to b2d5b3a Compare June 29, 2023 10:55
replace all instances of load_template_vars with get_template_vars

allow -z simple template variable string lists
@wxtim wxtim force-pushed the feature.plus_s_list_of_strings branch from b2d5b3a to eaae0bb Compare June 29, 2023 11:00
@oliver-sanders oliver-sanders requested review from MetRonnie and hjoliver and removed request for hjoliver and MetRonnie June 29, 2023 16:14
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

👍 thanks @wxtim

@hjoliver hjoliver merged commit 69be3f8 into cylc:master Jun 29, 2023
@wxtim wxtim deleted the feature.plus_s_list_of_strings branch July 4, 2023 09:39
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.

template vars: simpler syntax for defining a list of strings
3 participants