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: Control Rules #5052

Closed
wxtim opened this issue Aug 11, 2022 · 5 comments · Fixed by #5086
Closed

Lint: Control Rules #5052

wxtim opened this issue Aug 11, 2022 · 5 comments · Fixed by #5086
Assignees
Labels
speculative blue-skies ideas
Milestone

Comments

@wxtim
Copy link
Member

wxtim commented Aug 11, 2022

Description

Some style rules (such as jinja2 indentation) are widely and (perhaps) understandably violated -

This seems as legit

    [[section]]
        {% jinja2 stuff %}
             cylc key = cylc value
        {% end jinja2 stuff %}

as

    [[section]]
{% jinja2 stuff %}
        cylc key = cylc value
{% end jinja2 stuff %}

Which according to the rules is correct.

Possible solutions

  1. Claim that the linter is opinionated and leave it at that. This might be reasonable, given that disabling all rules affected will wreck any attempt to check indentation.
  2. Relax indentation rules - must be a multiple of 4. See 1 for objection.
  3. Add a command line switch to turn indent rules off.
  4. Add a command line switch to turn any specified rules off if the user wishes to.
  5. Add an rc file to do 4

Pull requests welcome!

@wxtim wxtim added the question Flag this as a question for the next Cylc project meeting. label Aug 11, 2022
@wxtim wxtim added this to the cylc-8.1.0 milestone Aug 11, 2022
@oliver-sanders
Copy link
Member

Note that it is not really possible to support the first indentation scheme in the issue as this would require a stateful linter but we have gone with a regex based approach.

Suggest: allowing the indentation rule to be turned off (any linter should allow rules to be selectively disabled IMO) and leaving it there.

@hdyson
Copy link

hdyson commented Aug 11, 2022

Can I propose another possible solution? Black allows ignoring sections of code, via fmt:off/fmt:on or fmt:skip comments: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#code-style An approach like this may be something to consider, since it lets someone say "We've made a deliberate decision that we're going to ignore the guidelines for a particular chunk of the code, but we still want to be warned about breaking the guidelines elsewhere"?

@wxtim
Copy link
Member Author

wxtim commented Aug 11, 2022

@hdyson I'd prefer not to litter workflows with extra directives.

We've had a bit of a discussion at team meeting and came to the conclusion that

a) We need to fix the index numbers of at least the style errors so that they remain consistent over time.
b) We should provide the ability to turn off individual linter items on the command line.
c) We should provide a per-workflow config file allowing project level control of the linter.

@wxtim wxtim self-assigned this Aug 11, 2022
@oliver-sanders oliver-sanders removed the question Flag this as a question for the next Cylc project meeting. label Aug 11, 2022
@wxtim
Copy link
Member Author

wxtim commented Aug 12, 2022

#5055 provides items a and b.

Item c will take a little more work, but:

Proposal for linter

Linter should be configurable on a per-workflow basis using a TOML file in the following format:

# .cylc-lint.toml
# Check against these rules
rulesets = [
    "728",
    "style"
]
#  do not check for these errors
ignore = [
    "S001"
]
# do not lint files matching
# these globs:
exclude = [
    "sites/*-niwa.flow",
    "*example.flow"
]
max-line-length = 79

Why TOML?

Simpler than YAML, easy to configure and parse.

@oliver-sanders
Copy link
Member

Note that isn't a toml parser in the standard library so this will involve adding a dependency (until pep0680).

The new "standard" way to configure tooling for Python projects is in a pyproject.toml file e.g. https://github.com/cylc/cylc-uiserver/blob/master/pyproject.toml.

@wxtim wxtim added the speculative blue-skies ideas label Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
speculative blue-skies ideas
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants