cylc-flow icon indicating copy to clipboard operation
cylc-flow copied to clipboard

Lint: Control Rules

Open wxtim opened this issue 3 years ago • 5 comments

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 avatar Aug 11 '22 08:08 wxtim

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.

oliver-sanders avatar Aug 11 '22 10:08 oliver-sanders

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"?

hdyson avatar Aug 11 '22 10:08 hdyson

@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 avatar Aug 11 '22 14:08 wxtim

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

wxtim avatar Aug 12 '22 08:08 wxtim

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.

oliver-sanders avatar Aug 12 '22 09:08 oliver-sanders