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

Allow CLI selection of a single rule to check. #6120

Closed
wants to merge 2 commits into from

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jun 4, 2024

Closes #5544

Additionally...

Fix a bug where empty flow.cylc files caused traceback.

To replicate

mkdir ~/cylc-src/foo
touch ~/cylc-src/foo/flow.cylc
cylc lint ~/cylc-src/foo

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
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim added this to the 8.3.1 milestone Jun 4, 2024
@wxtim wxtim self-assigned this Jun 4, 2024
@wxtim wxtim force-pushed the lint.allow_single_check_cli branch 2 times, most recently from a0ad10e to 23cc46f Compare June 4, 2024 09:08
@oliver-sanders
Copy link
Member

PR title and description don't appear to match?

Does this do both? If so maybe a bugfix changelog entry?

Fix a bug where empty flow.cylc files caused traceback.
@wxtim wxtim force-pushed the lint.allow_single_check_cli branch from 23cc46f to d235ca8 Compare June 5, 2024 11:55
@wxtim
Copy link
Member Author

wxtim commented Jun 5, 2024

Does this do both? If so maybe a bugfix changelog entry?

Yes. Sorry - original text not super clear. Have added the changelog entry as suggested.

Comment on lines 439 to 458
def log_iterable(
items: Iterable,
header: str,
singular_header: str,
) -> str:
"""Log a list.

Args:
items: The iterable
singular_header: To use if list has length 1
header: To use otherwise
"""
if len(items) == 0:
raise ValueError('This function does not take len==0 iterables')
elif len(items) == 1:
return singular_header.format(items[0])
else:
msg = header + '\n * '
msg += '\n * '.join(items)
return msg
Copy link
Member

@oliver-sanders oliver-sanders Jun 5, 2024

Choose a reason for hiding this comment

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

This function feels a bit over-complicated for a single use.

I think it should fail type checking as generators are iterables but are not supported by len()?

Not sure if mypy is smart enough to spot that?

Copy link
Member Author

@wxtim wxtim Jun 6, 2024

Choose a reason for hiding this comment

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

It is over complicated for a single use. I think that this is code we've replicated in a number of places, and may want again in future . The idea is that this function is available for refactoring elsewhere and for future use cases. Probably also needs list truncation logic too.

  • config.py, 570, 2584
  • platforms.py 700
  • rundb 938

You're right about the type hinting though: My local copy of MyPy has certainly picked that up.

@wxtim wxtim force-pushed the lint.allow_single_check_cli branch 2 times, most recently from 4f6d30e to dd0d00b Compare June 6, 2024 08:35
@wxtim wxtim requested a review from oliver-sanders June 6, 2024 08:36
@wxtim wxtim force-pushed the lint.allow_single_check_cli branch from dd0d00b to a172396 Compare June 6, 2024 08:43
@MetRonnie MetRonnie changed the base branch from master to 8.3.x June 19, 2024 12:33
@@ -1417,6 +1434,14 @@ def main(parser: COP, options: 'Values', target=None) -> None:
max_line_len=mergedopts[MAX_LINE_LENGTH]
)

if options.rule:
checks = {k: v for k, v in checks.items() if k in options.rule}
Copy link
Member

@oliver-sanders oliver-sanders Jun 27, 2024

Choose a reason for hiding this comment

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

I think this can be simplified to:

checks = {rule: checks[rule] for rule in options.rule}

Which has the advantage of raising an error if a rule does not exist.

Suggested change
checks = {k: v for k, v in checks.items() if k in options.rule}
try:
checks = {rule: checks[rule] for rule in options.rule}
except KeyError as exc:
LOG.error(f'No such rule {exc.args[0]}')
sys.exit(1)

However, it looks like --rule can only accept one rule at present? If so we don't need the dict comprehension.

I feel we should implement this more like --select (ruff/flake8), to allow the selection of rules OR rule sets?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rule can apply multiple rules:

cylc lint /var/tmp/cylc-src/mean.marble -R S001 -R S002

I feel we should implement this more like --select (ruff/flake8), to allow the selection of rules OR rule sets?

A legit point - I'll have a poke and see if I can do that without having two CLI args.
Should cut down the amount of code

parser.add_option(
'--rule', '-R',
help='Carry out only specific checks.',
choices=list(parse_checks(['style', '728']).keys()),
Copy link
Member

Choose a reason for hiding this comment

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

So --rule can be one of style or 728.

AND --ruleset can be one of style, 728 or all.

What's the point of --rule?

Copy link
Member

Choose a reason for hiding this comment

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

The --select / --ignore interface of ruff is a good gauge of how to go about implementing rule selection.

--select=R  # select all Rxxx rules
--select=R001  # select only R001
--select=R --ignore=R001  # select all R other than R001
--select=R001,R002  # select both R001 and R002
--select=ALL  # magic

Copy link
Member Author

@wxtim wxtim Jun 27, 2024

Choose a reason for hiding this comment

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

AND --ruleset can be one of style, 728 or all.

No - rulest can be any of the items in the parsed checks for both rulesets currently available because

>>> parse_checks(['style', '728'].keys()
DictKeys('S001', 'S002' ... ... 'R999')

Side effect of this is that the safety mechanism (try/except) is completely redundant.

@oliver-sanders oliver-sanders modified the milestones: 8.3.1, 8.3.x Jun 27, 2024
@wxtim wxtim marked this pull request as draft June 27, 2024 12:19
@wxtim
Copy link
Member Author

wxtim commented Jun 27, 2024

Redrafting - not a priority

@wxtim
Copy link
Member Author

wxtim commented Aug 19, 2024

Roll this into the full refactor of Cylc Lint - probably a good training day activity for Tim

@wxtim wxtim closed this Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cylc lint - command line option to only check specific options
2 participants