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

Lint: Tell users that backslashes are no longer necessary after => & | #6459

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Nov 5, 2024

Edit: superseded by #6493

Closes #6456

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).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • This is a feature, but a lint feature, so suitable to be added to a maintainance release.

@wxtim wxtim added the small label Nov 5, 2024
@wxtim wxtim requested a review from MetRonnie November 5, 2024 13:44
@wxtim wxtim self-assigned this Nov 5, 2024
@wxtim wxtim force-pushed the lint.6456.fewer_line_continuations branch from 6f7c711 to 4f3f8f1 Compare November 5, 2024 13:45
@wxtim wxtim modified the milestones: 8.3.6, 8.3.7 Nov 5, 2024
@oliver-sanders oliver-sanders requested review from markgrahamdawson and removed request for MetRonnie November 5, 2024 13:53
@wxtim wxtim requested a review from MetRonnie November 6, 2024 14:40
@oliver-sanders oliver-sanders removed the request for review from MetRonnie November 7, 2024 11:54
@wxtim wxtim requested a review from MetRonnie November 8, 2024 10:23
Comment on lines 724 to 729
'U017': {
'short': (
'`&` and `|` are line continuations without `\\`'
),
FUNCTION: re.compile(r'[&|]\s*\\').findall
},
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be part of S014

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that they should in time be, but at the moment they will not work if you are using compat mode Cylc 8 and Cylc 7 in Para||el, so I think that they should be an upgrade until Cylc 7 is more dead than it currently is.

Copy link
Member

Choose a reason for hiding this comment

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

"more dead" 😁

changes.d/6459.feat.md Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from MetRonnie November 19, 2024 09:55
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.

One typo detected and a couple of small tweaks suggested.

(It may be a bit picky, but I'd say "=>" etc., imply line continuation rather than being line continuation).

changes.d/6459.feat.md Outdated Show resolved Hide resolved
cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
Co-authored-by: Hilary James Oliver <[email protected]>
@wxtim wxtim requested a review from hjoliver November 22, 2024 08:33
@wxtim wxtim merged commit ee20614 into cylc:8.3.x Nov 22, 2024
27 checks passed
@MetRonnie
Copy link
Member

This has added a new lint rule S014 on 8.3.x, however S014 is already a different rule on master (#6137), blocking sync PRs for the time being (#6489). I think this should be reverted and re-opened against master.

@wxtim wxtim deleted the lint.6456.fewer_line_continuations branch November 25, 2024 11:09
@wxtim wxtim restored the lint.6456.fewer_line_continuations branch November 25, 2024 11:11
@MetRonnie MetRonnie removed this from the 8.3.7 milestone Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants