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

Cylc 728 (with linter) #4900

Merged
merged 59 commits into from
Jul 27, 2022
Merged

Cylc 728 (with linter) #4900

merged 59 commits into from
Jul 27, 2022

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented May 27, 2022

Work in Progress

Posted as a PR for discussion on whether the approach seems OK - it's slightly over-engineered on the basis that it could become a Cylc Linter with very little work.

These changes close #4519

Requirements 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.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • (master branch) I have opened a documentation PR at Document cylc lint cylc-doc#480.

@wxtim wxtim marked this pull request as draft May 27, 2022 10:17
@wxtim wxtim self-assigned this May 27, 2022
@wxtim wxtim requested review from datamel and MetRonnie May 27, 2022 12:52
@wxtim wxtim added this to the cylc-8.0.0 milestone May 27, 2022
@wxtim wxtim force-pushed the cylc-linter branch 2 times, most recently from 77871a1 to aa4e933 Compare May 27, 2022 13:01
@MetRonnie
Copy link
Member

Looks good 👍 only thing is we should probably exclude the log directory

cylc/flow/scripts/lint.py 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
@wxtim wxtim marked this pull request as ready for review May 31, 2022 16:06
@oliver-sanders
Copy link
Member

oliver-sanders commented Jun 8, 2022

Would probably call the command 728 rather than linter since it's upgrading not linting (we had a cylc 526 command in the past). Would be good to leave the door open to a linter one day.

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 have tried this on

[cylc]
    UTC mode = True
[scheduling]
    initial cycle point = 2000-01-01T00Z
    final cycle point = 2000-01-04T00Z
    [[dependencies]]
        [[[PT3H]]]
            graph = """
                get_observations_belmullet & get_observations_camborne & get_observations_heathrow & get_observations_shetland => consolidate_observations
            """

expecting it to pick up on [cylc] and graph =, but instead it just says

Checked cylc7-datetime-cycling/suite.rc and found 0 issues.

cylc/flow/scripts/lint.py 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
cylc/flow/scripts/lint.py 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
cylc/flow/scripts/lint.py 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
cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
@wxtim
Copy link
Member Author

wxtim commented Jun 16, 2022

I have tried this on

[cylc]
    UTC mode = True
[scheduling]
    initial cycle point = 2000-01-01T00Z
    final cycle point = 2000-01-04T00Z
    [[dependencies]]
        [[[PT3H]]]
            graph = """
                get_observations_belmullet & get_observations_camborne & get_observations_heathrow & get_observations_shetland => consolidate_observations
            """

expecting it to pick up on [cylc] and graph =, but instead it just says

Checked cylc7-datetime-cycling/suite.rc and found 0 issues.

I cannot replicate this - does it still happen?

I get

> cylc 728 
[001: 7-to-8]/net/home/h02/tpilling/cylc-src/temp/flow.cylc: 0: section ``[cylc]`` is now called ``[scheduler]``.
[021: 7-to-8]/net/home/h02/tpilling/cylc-src/temp/flow.cylc: 5: ``[dependencies]`` is deprecated.
[022: 7-to-8]/net/home/h02/tpilling/cylc-src/temp/flow.cylc: 7: ``[cycle point]graph =`` is deprecated, use ``cycle point = <graph>``
Checked /net/home/h02/tpilling/cylc-src/temp and found 3 issues.

@wxtim wxtim changed the title Cylc linter Cylc 728 (with linter) Jun 16, 2022
@wxtim wxtim requested a review from MetRonnie June 16, 2022 09:25
@wxtim wxtim mentioned this pull request Jun 16, 2022
1 task
@oliver-sanders oliver-sanders mentioned this pull request Jun 16, 2022
14 tasks
@wxtim
Copy link
Member Author

wxtim commented Jun 16, 2022

@MetRonnie - I showed @oliver-sanders the fact that I've effectively already written a linter and he got a bit excited and made a fair few suggestions. I've implemented a couple so this PR has changed again.

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.

Partial review done @wxtim but there's a bunch of spurious changes that already exist on master - might be a rebase required?

CHANGES.md Outdated Show resolved Hide resolved
conda-environment.yml Outdated Show resolved Hide resolved
cylc/flow/rundb.py Outdated Show resolved Hide resolved
cylc/flow/task_proxy.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from hjoliver June 17, 2022 09:00
@wxtim
Copy link
Member Author

wxtim commented Jun 17, 2022

@hjoliver - I think that I must have botched a rebase - I think that's now fixéd.

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.

Linting works, however the line numbers are 1 lower than they should be

CHANGES.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
cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
cylc/flow/scripts/lint.py 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
cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
@oliver-sanders
Copy link
Member

Linting works, however the line numbers are 1 lower than they should be

This may be due to the shebang line? If so, note this may or may not be present.

@wxtim wxtim requested a review from MetRonnie June 22, 2022 15:07
@wxtim
Copy link
Member Author

wxtim commented Jun 22, 2022

Linting works, however the line numbers are 1 lower than they should be

required a start=1 kwarg to an enumerate()

@MetRonnie
Copy link
Member

MetRonnie commented Jun 22, 2022

@wxtim Flake8 failure (and while you're at it could you squash down these Update cylc/flow/scripts/lint.py commits?)

@wxtim wxtim force-pushed the cylc-linter branch 2 times, most recently from 6623ebb to f171c72 Compare June 24, 2022 14:02
@MetRonnie
Copy link
Member

MetRonnie commented Jun 24, 2022

I'm getting spurious changes coming through in f171c72 (undoing recent commits on master). This is my effort at rebasing: https://github.com/MetRonnie/cylc-flow/tree/tmp. If you're happy with it could you reset your branch to the tip of that?

(Converting to draft so as not to merge with these reversions)

@MetRonnie MetRonnie marked this pull request as draft June 24, 2022 16:17
wxtim and others added 2 commits July 26, 2022 08:06
- Delete the examples section of the docstring as per HO suggestion

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
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.

Now it is running 728 checks on suite.rc workflows

@oliver-sanders
Copy link
Member

Found two small issues (sorry):

  1. When linting multiple workflows the number of reported issues is carried over from previous workflows to future ones.
  2. The [runtime][<namespace>][environment] section is being erroneously flagged as the obsoleted [cylc][environment] section.

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.

Formatting issue

$ cylc lint simple-c7-datetime/
Checked ~/cylc-run/simple-c7-datetime against style checksFound no issues.
$ #                                                       ^ missing punctuation

@wxtim wxtim requested a review from MetRonnie July 27, 2022 07:02
Comment on lines 495 to 496
f'Checked {target} against {check_names} checks'
f'rules and found {count} issue'
f'rules and found {count} issue(s).'
Copy link
Member

Choose a reason for hiding this comment

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

"checksrules"

Checked {target} against {check_names} checksrules and found {count} issue(s).

cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from MetRonnie July 27, 2022 09:34
cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
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.

Tested, happy my review comments have been addressed.

(please squash this one when you merge 😁)

wxtim and others added 2 commits July 27, 2022 12:23
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
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.

🎇🎊🎉

@MetRonnie MetRonnie merged commit 1fad73f into cylc:master Jul 27, 2022
@wxtim wxtim deleted the cylc-linter branch December 1, 2022 13:14
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.

basic cylc 8 syntax checker to assist upgrade
5 participants