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

pyproject.toml: cylc lint settings #5086

Merged

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Aug 18, 2022

Closes #5052

Check List

@wxtim wxtim force-pushed the allow_pyproject.toml_with_cylc_lint_settings branch 2 times, most recently from 35eabf0 to 68529c9 Compare August 18, 2022 07:55
@wxtim wxtim self-assigned this Aug 18, 2022
@wxtim wxtim added this to the cylc-8.1.0 milestone Aug 18, 2022
@wxtim wxtim requested review from MetRonnie, hjoliver and oliver-sanders and removed request for MetRonnie August 18, 2022 08:09
@wxtim
Copy link
Member Author

wxtim commented Aug 18, 2022

Waiting for reviewers to check that I've not gone off at teh deep end prior to writing a documentation PR

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.

Partial review

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
@MetRonnie
Copy link
Member

1 doctest failure

@oliver-sanders oliver-sanders removed the request for review from hjoliver August 23, 2022 08:36
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.

Partial review

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
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.

Tested, working 👍

Please could you add a minimal example pyproject.toml example to the PR

tests/unit/scripts/test_lint.py Outdated Show resolved Hide resolved
tests/unit/scripts/test_lint.py Show resolved Hide resolved
tests/unit/scripts/test_lint.py Outdated Show resolved Hide resolved
cylc/flow/scripts/lint.py Show resolved Hide resolved
@MetRonnie
Copy link
Member

Oops, tests/f/cylc-lint/01.lint-toml.t failing

@wxtim wxtim force-pushed the allow_pyproject.toml_with_cylc_lint_settings branch from c494ef1 to 0080c82 Compare August 30, 2022 09:01
@wxtim wxtim changed the title allow pyproject.toml with cylc lint settings pyproject.toml: cylc lint settings Sep 9, 2022
@wxtim wxtim requested a review from oliver-sanders September 9, 2022 07:59
@wxtim wxtim force-pushed the allow_pyproject.toml_with_cylc_lint_settings branch 2 times, most recently from 03150c4 to 0a6fbc0 Compare September 9, 2022 09:07
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.

I got some surprising behaviour trying to ignore a code:

[cylc-lint]              
    max-line-length = 20    
    ignore = ['S008']  
$ cylc lint
CylcError: S008 is a not a known linter code.

And there appears to be a missing example in the CLI docs, otherwise fine.

cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
@wxtim wxtim force-pushed the allow_pyproject.toml_with_cylc_lint_settings branch from ac1cfd2 to c6952ca Compare September 13, 2022 07:37
cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
Comment on lines 392 to 398
def parse_checks(check_args, ignores=None, max_line_len=130, reference=False):
"""Collapse metadata in checks dicts.

Args:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what "Collapse metadata in checks dicts" means.

The reference arg is missing a docstring entry

Copy link
Member

Choose a reason for hiding this comment

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

Is 130 the default max_line_len?

Copy link
Member Author

@wxtim wxtim Sep 13, 2022

Choose a reason for hiding this comment

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

Yes, is that a sensible number - I wanted to make it a long but not insane length. I could instead make it v.v. big.

Setting a default meant that the problems you'd been having with it not appearing in the lists of rules went away.

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.

ignore = ['S008'] is now recognised but does not work

Comment on lines 423 to 582
argdoc=[COP.optional(('DIR ...', 'Directories to lint'))],
argdoc=[COP.optional(WORKFLOW_ID_OR_PATH_ARG_DOC)],
Copy link
Member

Choose a reason for hiding this comment

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

The main function is not set up to handle workflow IDs, however. It is treating the ID as a relative path. Also I get traceback following this

$ cylc lint foo
2022-09-13T14:49:30+01:00 WARNING - Path foo does not exist.
Traceback (most recent call last):
  File "~/.conda/envs/cylc8/bin/cylc", line 33, in <module>
    sys.exit(load_entry_point('cylc-flow', 'console_scripts', 'cylc')())
  File "~/github/cylc-flow/cylc/flow/scripts/cylc.py", line 657, in main
    execute_cmd(command, *cmd_args)
  File "~/github/cylc-flow/cylc/flow/scripts/cylc.py", line 284, in execute_cmd
    entry_point.resolve()(*args)
  File "~/github/cylc-flow/cylc/flow/terminal.py", line 229, in wrapper
    wrapped_function(*wrapped_args, **wrapped_kwargs)
  File "~/github/cylc-flow/cylc/flow/scripts/lint.py", line 718, in main
    f'Checked {target} against {check_names} rules and '
UnboundLocalError: local variable 'check_names' referenced before assignment

@wxtim
Copy link
Member Author

wxtim commented Sep 14, 2022

ignore = ['S008'] is now recognised but does not work

I can't replicate this - can you send me the toml file you are using - It took me a wee while to remember that my section heading was cylc-lint and not cylc_lint or cylclint. What do you think of testing for any of those?

@MetRonnie
Copy link
Member

Just using

[cylc-lint]
    ignore = ['S008']

and having a line longer than the default 130

@MetRonnie
Copy link
Member

It took me a wee while to remember that my section heading was cylc-lint and not cylc_lint or cylclint. What do you think of testing for any of those?

I think it's too much work to try and anticipate users' typos

@wxtim wxtim marked this pull request as draft September 14, 2022 08:28
* allow pyproject.toml with cylc lint settings
Co-authored-by: Ronnie Dutta <[email protected]>

response to review

Made cylc lint accept only one argument.
@wxtim wxtim force-pushed the allow_pyproject.toml_with_cylc_lint_settings branch from 0f8b7c6 to 439df2e Compare September 14, 2022 08:36
@wxtim
Copy link
Member Author

wxtim commented Sep 14, 2022

It took me a wee while to remember that my section heading was cylc-lint and not cylc_lint or cylclint. What do you think of testing for any of those?

I think it's too much work to try and anticipate users' typos

I see - I think I've fixed it

@wxtim wxtim marked this pull request as ready for review September 14, 2022 08:37
@MetRonnie
Copy link
Member

Everythging seems to be working apart from ignoring S008. I suggest adding a test for that too

@wxtim wxtim marked this pull request as draft September 14, 2022 11:47
@wxtim
Copy link
Member Author

wxtim commented Sep 14, 2022

Everythging seems to be working apart from ignoring S008. I suggest adding a test for that too

Reverted to draft - not priority this afternboon

@wxtim wxtim marked this pull request as ready for review September 15, 2022 13:29
@wxtim wxtim requested a review from MetRonnie September 26, 2022 08:57
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.

Thanks!

@oliver-sanders oliver-sanders merged commit 88c20a7 into cylc:master Oct 3, 2022
@wxtim wxtim deleted the allow_pyproject.toml_with_cylc_lint_settings branch October 3, 2022 12:42
datamel pushed a commit to datamel/cylc-flow that referenced this pull request Oct 19, 2022
lint: allow pyproject.toml with cylc lint settings (cylc#46)

* Support pyproject.toml with cylc lint settings.
* Made cylc lint accept only one argument.
* Made sure that line length checks are ignored when added to the ignore list.
* Remove now unused check
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.

Lint: Control Rules
3 participants