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

Fix S011 lint #5536

Merged
merged 3 commits into from
May 17, 2023
Merged

Fix S011 lint #5536

merged 3 commits into from
May 17, 2023

Conversation

ColemanTom
Copy link
Contributor

@ColemanTom ColemanTom commented May 15, 2023 •
edited
Loading

The linter suggests that Cylc parses Jinja2 when it is commneted, which I agree with. However if it is a Jinja2 comment wrapping Jinja2 , then it will parse the comment and ignore the inside. It should not be a warning.

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

@MetRonnie MetRonnie changed the base branch from master to 8.1.x May 15, 2023 14:32
@MetRonnie
Copy link
Member

Thanks @ColemanTom

Can you rebase this commit onto 8.1.x please?

@MetRonnie MetRonnie added this to the cylc-8.1.x milestone May 15, 2023
@MetRonnie MetRonnie added small could be better Not exactly a bug, but not ideal. labels May 15, 2023
@ColemanTom
Copy link
Contributor Author

Thanks @ColemanTom

Can you rebase this commit onto 8.1.x please?

Ok. I didn't notice it was automatically going against 8.1.x. The CONTRIBUTING.md says to make all PR against master, so that is what I had branched off of.

@ColemanTom
Fix S011 lint
62a99b0 
Cylc does not parse Jinja2 when inside a Jinja2 comment.
@oliver-sanders
Copy link
Member

oliver-sanders commented May 16, 2023 •
edited
Loading

Ok. I didn't notice it was automatically going against 8.1.x. The CONTRIBUTING.md says to make all PR against master, so that is what I had branched off of.

Sorry, not the best explained. We use a semver approach to avoid letting risky changes through in bugfix releases.

  • Bugfixes and minor low-risk usability improvements can go onto the bugfix branch (currently 8.1.x) for the next bugfix release (8.1.5). Generally "bug" or "could-be-better" issues go into bugfix releases.
  • Enhancements, refactors and other things should go onto master for the next minor release (8.2.0).

For something like this there's very little risk so we might as well get it into 8.1.5 rather than waiting around for 8.2.0.

(note, we routinely merge the bugfix branch into master to pull in these bugfixes)

@oliver-sanders oliver-sanders requested review from wxtim and MetRonnie May 16, 2023 10:36
@oliver-sanders oliver-sanders modified the milestones: cylc-8.1.x, cylc-8.1.5 May 16, 2023
wxtim
wxtim approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

Ronnie's Change is probably a good idea, otherwise, I'm happy with this.

@oliver-sanders
Copy link
Member

FYI: I've added some more detail to the CONTRIBUTING here: #5548

@ColemanTom @MetRonnie
Improve test_check_cylc_file_jinja2_comments
5a028b7 
Adopt suggestion from @MetRonnie to reduce fragility.

Co-authored-by: Ronnie Dutta <[email protected]>
hjoliver
hjoliver approved these changes May 16, 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.

Looks good. One faintly off-piste question for the dev team.

@oliver-sanders @hjoliver
Update cylc/flow/scripts/lint.py
e35280d 
Co-authored-by: Hilary James Oliver <[email protected]>
@oliver-sanders oliver-sanders merged commit a1f9398 into cylc:8.1.x May 17, 2023
@MetRonnie MetRonnie mentioned this pull request May 17, 2023
Merged
8 tasks
wxtim added a commit to wxtim/cylc that referenced this pull request May 19, 2023
@wxtim
Merge branch 'fix.sort_lint_listing--correct_number_for_line_len' of …
7fe4ac9 
…github.com:wxtim/cylc into fix.sort_lint_listing--correct_number_for_line_len

* 'fix.sort_lint_listing--correct_number_for_line_len' of github.com:wxtim/cylc:
  `cylc play`: add suggestion for `--upgrade` in non-interactive terminal (cylc#5535)
  centralize number used for line length check.
  Respond to review
  Fix cylc lint commented-out Jinja2 bug
  Logging: say command actioned instead of succeeded
  cylc lint non zero code from warnings (cylc#5546)
  Fix S011 lint (cylc#5536)
@hjoliver hjoliver modified the milestones: cylc-8.1.5, cylc-8.2.0 Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxtim wxtim wxtim approved these changes

@hjoliver hjoliver hjoliver approved these changes

@MetRonnie MetRonnie Awaiting requested review from MetRonnie

Assignees

@ColemanTom ColemanTom

Labels
could be better Not exactly a bug, but not ideal. small
Projects
None yet
Milestone
cylc-8.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ColemanTom @MetRonnie @oliver-sanders @hjoliver @wxtim