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

Raise error if "=" not in define #225

Merged
merged 4 commits into from
Oct 16, 2023
Merged

Raise error if "=" not in define #225

merged 4 commits into from
Oct 16, 2023

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented May 4, 2023 •
edited

Closes #226

Built atop #221

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
  • No documentation update req'd
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim requested review from MetRonnie and hjoliver May 4, 2023 12:49
@wxtim wxtim self-assigned this May 4, 2023
@wxtim wxtim modified the milestones: 1.2.x, 1.2.1 May 4, 2023
@wxtim wxtim force-pushed the feature.do_not_allow_--defines_no_equal branch 3 times, most recently from 9067608 to 5067143 Compare May 4, 2023 13:55
@hjoliver
Copy link
Member

hjoliver commented May 5, 2023 •
edited

Closes #2691

Wrong Issue number? [link was dead]

@wxtim wxtim changed the title Feature.do not allow defines no equal Error if "=" not in define May 5, 2023
@wxtim
Copy link
Member Author

wxtim commented May 5, 2023

Closes #2691

Wrong Issue number?

I originally wrote this up as a rose issue (because it seems like a rose problem).

@MetRonnie MetRonnie changed the title Error if "=" not in define Raise error if "=" not in define May 5, 2023
hjoliver
hjoliver reviewed May 8, 2023
View reviewed changes
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.

Couple of minor questions.

cylc/rose/entry_points.py Outdated Show resolved Hide resolved
cylc/rose/utilities.py Show resolved Hide resolved
cylc/rose/utilities.py Outdated Show resolved Hide resolved
cylc/rose/utilities.py Outdated

if match['state'] in ['!', '!!']:
LOG.warning(
'CLI opts set to ignored or trigger-ignored will be ignored.')
Copy link
Member

Choose a reason for hiding this comment

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

Is that a thing? (Use the ignore syntax on the command line?) Why would anyone do that? And if they do do it, should it need a warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that a thing? (Use the ignore syntax on the command line?) Why would anyone do that?

@oliver-sanders - Like Hilary, I have no idea why anyone would do this - do you know?

And if they do do it, should it need a warning?

There's already a warning in the post install entry point, and perhaps it only needs to be there if it's only a warning.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is a "thing", in Rose ignored items still exist in the configuration, just with a different state so could still be functional in some strange way. A good example of this is the opts config in the rose-suite-cylc-install.conf file which is marked as ignored (for clarity because opts are not functional when defined in optional configs) but this value is used by the cylc-rose plugin to cylc reinstall so is functional.

I don't think we need a warning here, you'd have to go out of your way to add an ignored config, if you do we can only assume you know what you're doing.

wxtim
wxtim commented May 9, 2023
View reviewed changes
cylc/rose/utilities.py Outdated Show resolved Hide resolved
@wxtim wxtim linked an issue Jun 29, 2023 that may be closed by this pull request
Error if "=" not in define #226
Closed
@oliver-sanders oliver-sanders removed the request for review from hjoliver July 17, 2023 09:39
@oliver-sanders oliver-sanders modified the milestones: 1.2.1, 1.3.1 Jul 17, 2023
@MetRonnie

This comment was marked as resolved.

Sign in to view
@wxtim wxtim marked this pull request as draft July 17, 2023 14:15
@wxtim wxtim marked this pull request as ready for review July 17, 2023 14:22
@wxtim wxtim marked this pull request as draft July 17, 2023 14:47
@oliver-sanders oliver-sanders changed the base branch from 1.2.x to 1.3.x August 8, 2023 12:30
@wxtim wxtim force-pushed the feature.do_not_allow_--defines_no_equal branch 2 times, most recently from 29256a6 to 7e5414f Compare October 9, 2023 13:07
@wxtim wxtim marked this pull request as ready for review October 9, 2023 13:07
@wxtim wxtim force-pushed the feature.do_not_allow_--defines_no_equal branch from 8c4e015 to bcfc149 Compare October 9, 2023 13:39
@wxtim wxtim force-pushed the feature.do_not_allow_--defines_no_equal branch from bcfc149 to 5d4d389 Compare October 9, 2023 13:45
@wxtim
Merge branch '1.3.x' into feature.do_not_allow_--defines_no_equal
1ade23c
@wxtim
remove back compatibility check
d35f74f
@wxtim
Merge branch 'feature.do_not_allow_--defines_no_equal' of github.com:…
c724d8b 
…wxtim/cylc-rose into feature.do_not_allow_--defines_no_equal

* 'feature.do_not_allow_--defines_no_equal' of github.com:wxtim/cylc-rose:
  Ensure that a `:suite.rc` is added to the template language when creating defines.
@oliver-sanders oliver-sanders removed the request for review from hjoliver October 13, 2023 08:40
@hjoliver hjoliver merged commit dd24b82 into cylc:1.3.x Oct 16, 2023
5 checks passed
@wxtim wxtim deleted the feature.do_not_allow_--defines_no_equal branch October 17, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjoliver hjoliver hjoliver approved these changes

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

@MetRonnie MetRonnie Awaiting requested review from MetRonnie

Assignees

@wxtim wxtim

Labels
None yet
Projects
None yet
Milestone
1.3.1
Development

Successfully merging this pull request may close these issues.

Error if "=" not in define
4 participants
@wxtim @hjoliver @MetRonnie @oliver-sanders