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.

Sign up for GitHub

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

Jump to bottom

Support expanding <param1><param2> in inherited family names #5537

Merged
merged 3 commits into from
Jun 29, 2023
Merged

Support expanding <param1><param2> in inherited family names #5537

merged 3 commits into from
Jun 29, 2023

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented May 15, 2023 •
edited by MetRonnie
Loading

Closes #5118

The form FAM<foo,bar> is already supported in graph strings and [runtime][task]inherit, while the functionally equivalent FAM<foo><bar> is only supported in graph strings. This PR adds full support for the latter

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
  • Cylc-Doc pull request opened if required at added documentation for <param><param2> syntax cylc-doc#611.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch. N/A

@wxtim wxtim force-pushed the feature.support_multiple_inheriting_parameterized_families.control_test branch from 95ad6d9 to 11b4cca Compare May 15, 2023 12:38
@oliver-sanders oliver-sanders added this to the cylc-8.2.0 milestone May 15, 2023
oliver-sanders
oliver-sanders reviewed May 16, 2023
View reviewed changes
cylc/flow/param_expand.py Outdated Show resolved Hide resolved
cylc/flow/param_expand.py Outdated Show resolved Hide resolved
@oliver-sanders oliver-sanders mentioned this pull request May 16, 2023
Open
14 tasks
@hjoliver
Copy link
Member

hjoliver commented May 16, 2023 •
edited
Loading

I'm not clear if we need to document it since many (most?) users seem to think that it exists regardless?

I think it needs to be documented now that's it's becoming officially supported syntax. All examples in the docs will (presumably) be using the original syntax.

@wxtim
Copy link
Member Author

wxtim commented May 17, 2023

I'm not clear if we need to document it since many (most?) users seem to think that it exists regardless?

I think it needs to be documented now that's it's becoming officially supported syntax. All examples in the docs will (presumably) be using the original syntax.

Coming back to this after a couple of days I can't believe I said this!

@wxtim wxtim marked this pull request as draft May 17, 2023 07:19
@wxtim
Copy link
Member Author

wxtim commented May 17, 2023

Converted to draft until I've:

  • Responded to comments
  • Written a docs PR

@wxtim wxtim mentioned this pull request May 18, 2023
Merged
1 task
@wxtim wxtim requested a review from oliver-sanders May 18, 2023 11:50
@wxtim wxtim marked this pull request as ready for review May 18, 2023 11:51
@wxtim wxtim marked this pull request as draft June 5, 2023 08:29
@wxtim
Copy link
Member Author

wxtim commented Jun 5, 2023 •
edited
Loading

Converted to draft whilst I look into using the existing "expand" method.

I had a look at using the expand method, which should in theory be a shoe-in fit - but isn't - I've spent 2 working days trying various methods for making it work. I'll have another look at some point (because really), but I'm burnt out on it now and this approach works. I've included what I've done in this branch.

@wxtim wxtim marked this pull request as ready for review June 6, 2023 07:14
@MetRonnie MetRonnie requested review from MetRonnie and removed request for hjoliver June 21, 2023 09:27
MetRonnie
MetRonnie reviewed Jun 21, 2023
View reviewed changes
CHANGES.md Outdated Show resolved Hide resolved
tests/unit/test_param_expand.py Outdated Show resolved Hide resolved
MetRonnie
MetRonnie reviewed Jun 21, 2023
View reviewed changes
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

I've unresolved some of Oliver's comments which have not been addressed

tests/unit/test_param_expand.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from MetRonnie June 22, 2023 10:17
MetRonnie
MetRonnie reviewed Jun 22, 2023
View reviewed changes
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

I've noticed that <x>foo<y> ends up like _x1foo_y1, though I imagine this is a low priority issue

cylc/flow/param_expand.py Outdated Show resolved Hide resolved
cylc/flow/param_expand.py Outdated Show resolved Hide resolved
tests/unit/test_param_expand.py Outdated Show resolved Hide resolved
@wxtim
Allow the full parameter expansion of [runtime][namespace].
4abf220 
This includes the following, not previously permitted pattern:
`familyname<foo><bar>` and `family<foo>name<bar>`.
@wxtim wxtim force-pushed the feature.support_multiple_inheriting_parameterized_families.control_test branch from cb47f4f to 4abf220 Compare June 23, 2023 07:54
@wxtim wxtim requested a review from MetRonnie June 23, 2023 07:55
MetRonnie
MetRonnie previously approved these changes Jun 23, 2023
View reviewed changes
@wxtim wxtim force-pushed the feature.support_multiple_inheriting_parameterized_families.control_test branch from 5ad9503 to 4abf220 Compare June 27, 2023 14:21
@MetRonnie MetRonnie dismissed their stale review June 27, 2023 15:38

Suggestions were reverted

@wxtim wxtim requested a review from MetRonnie June 29, 2023 10:08
MetRonnie
MetRonnie requested changes Jun 29, 2023
View reviewed changes
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

#5537 (comment)

@wxtim wxtim requested a review from MetRonnie June 29, 2023 12:22
@oliver-sanders oliver-sanders merged commit 7b240d1 into cylc:master Jun 29, 2023
@wxtim wxtim deleted the feature.support_multiple_inheriting_parameterized_families.control_test branch July 4, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MetRonnie MetRonnie MetRonnie approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@wxtim wxtim

Labels
None yet
Projects
None yet
Milestone
cylc-8.2.0
Development

Successfully merging this pull request may close these issues.

parameters: complete support for alternative syntax
4 participants
@wxtim @hjoliver @oliver-sanders @MetRonnie