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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes.d/6120.bug.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug causing ``cylc lint`` to exit with traceback if given an empty file.
1 change: 1 addition & 0 deletions changes.d/6120.feat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow checking of individual ``cylc lint`` checks using ``--rule`` (or ``-R``)
25 changes: 24 additions & 1 deletion cylc/flow/loggingutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import re
import sys
import textwrap
from typing import List, Optional, Union
from typing import Iterable, List, Optional, Union

Check warning on line 34 in cylc/flow/loggingutil.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/loggingutil.py#L34

Added line #L34 was not covered by tests

from ansimarkup import parse as cparse, strip as cstrip

Expand Down Expand Up @@ -434,3 +434,26 @@
logger.setLevel(orig_level)
else: # No need to patch
yield


def bullet_list(

Check warning on line 439 in cylc/flow/loggingutil.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/loggingutil.py#L439

Added line #L439 was not covered by tests
items: Iterable[str],
header: str,
singular_header: str,
) -> str:
"""Create a bullet point list of items.

Args:
items: The iterable
singular_header: To use if list has length 1
header: To use otherwise
"""
items = list(items)

Check warning on line 451 in cylc/flow/loggingutil.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/loggingutil.py#L451

Added line #L451 was not covered by tests
if len(items) == 0:
raise ValueError('This function does not take len==0 iterables')

Check warning on line 453 in cylc/flow/loggingutil.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/loggingutil.py#L453

Added line #L453 was not covered by tests
elif len(items) == 1:
return singular_header.format(items[0])

Check warning on line 455 in cylc/flow/loggingutil.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/loggingutil.py#L455

Added line #L455 was not covered by tests
else:
msg = header + '\n * '
msg += '\n * '.join(items)
return msg

Check warning on line 459 in cylc/flow/loggingutil.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/loggingutil.py#L457-L459

Added lines #L457 - L459 were not covered by tests
29 changes: 27 additions & 2 deletions cylc/flow/scripts/lint.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
from cylc.flow import LOG
from cylc.flow.exceptions import CylcError
import cylc.flow.flags
from cylc.flow.loggingutil import set_timestamps
from cylc.flow.loggingutil import set_timestamps, bullet_list
from cylc.flow.option_parsers import (
CylcOptionParser as COP,
WORKFLOW_ID_OR_PATH_ARG_DOC
Expand Down Expand Up @@ -1144,7 +1144,10 @@
"""
# get the first line
line_no = 1
line = next(lines)
try:
line = next(lines)
except StopIteration:
return

Check warning on line 1150 in cylc/flow/scripts/lint.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/scripts/lint.py#L1149-L1150

Added lines #L1149 - L1150 were not covered by tests
# check if it is a jinja2 shebang
jinja_shebang = line.strip().lower() == JINJA2_SHEBANG

Expand Down Expand Up @@ -1340,6 +1343,13 @@
action='store_true',
default=False,
)
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.

action='append',
default=[],
)
parser.add_option(
'--ruleset', '-r',
help=(
Expand Down Expand Up @@ -1390,6 +1400,13 @@
print(get_reference(options.ruleset, 'text'))
sys.exit(0)

if options.rule and options.ruleset:
LOG.error('arguments --rule and --ruleset are mutually exclusive')
sys.exit(1)

Check warning on line 1405 in cylc/flow/scripts/lint.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/scripts/lint.py#L1404-L1405

Added lines #L1404 - L1405 were not covered by tests
if options.rule and options.ignores:
LOG.error('arguments --rule and --ignores are mutually exclusive')
sys.exit(1)

Check warning on line 1408 in cylc/flow/scripts/lint.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/scripts/lint.py#L1407-L1408

Added lines #L1407 - L1408 were not covered by tests

# If target not given assume we are looking at PWD:
if target is None:
target = str(Path.cwd())
Expand Down Expand Up @@ -1417,6 +1434,14 @@
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

LOG.warning(bullet_list(
[f'{k}: {v["short"]}' for k, v in checks.items()],
header='Checking only:',
singular_header='Checking only {}')
)

# Check each file matching a pattern:
counter: Dict[str, int] = {}
for file in get_cylc_files(target, mergedopts[EXCLUDE]):
Expand Down
88 changes: 88 additions & 0 deletions tests/integration/scripts/test_lint.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
#!/usr/bin/env python3
# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.

# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
"""Tests `cylc lint` CLI Utility.

TODO: Consider tests in unit test file for movement to here.
"""

import pytest

from cylc.flow.scripts.lint import main, get_option_parser

cylc_lint = main.__wrapped__


@pytest.fixture
def setup():
parser = get_option_parser()
options = parser.get_default_values()
return parser, options


def test_lint_empty_file(tmp_path, setup, caplog):
"""Argument --rule is mutually exclusive of either --ignores
and --ruleset.
"""
(tmp_path / 'flow.cylc').touch()
parser, options = setup
with pytest.raises(SystemExit, match="False"):
main.__wrapped__(parser, options, str(tmp_path))


def test_mutually_exclusive_args(tmp_path, setup):
"""Argument --rule is mutually exclusive of either --ignores
and --ruleset.
"""
(tmp_path / 'flow.cylc').write_text('[hi]')
parser, options = setup
options.rule = ['S002']

# Rule + Ruleset
options.ruleset = '728'
with pytest.raises(SystemExit, match="1"):
main.__wrapped__(parser, options, str(tmp_path))

# Rule + Ignores
options.ignores = ['S005']
options.ruleset = ''
with pytest.raises(SystemExit, match="1"):
main.__wrapped__(parser, options, str(tmp_path))


def test_single_check_cli(setup, tmp_path, caplog):
"""Demo that CLI --rule option works.
"""
(tmp_path / 'flow.cylc').write_text(' [meta]')
parser, options = setup

# This rule should cause a failure:
options.rule = ['S003']
with pytest.raises(SystemExit, match="True"):
main.__wrapped__(parser, options, str(tmp_path))
assert (
'Checking only S003: Top level sections should'
' not be indented.'
) in caplog.messages

# This rule should NOT cause a failure:
options.ruleset = ''
options.rule = ['S001']
with pytest.raises(SystemExit, match="False"):
main.__wrapped__(parser, options, str(tmp_path))
assert (
'Checking only S001: Use multiple spaces, not tabs'
) in caplog.messages
15 changes: 15 additions & 0 deletions tests/unit/test_loggingutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
CylcLogFormatter,
get_reload_start_number,
get_sorted_logs_by_time,
bullet_list,
set_timestamps,
)

Expand Down Expand Up @@ -197,3 +198,17 @@ def test_set_timestamps(capsys):
assert re.match('^[0-9]{4}', errors[0])
assert re.match('^WARNING - bar', errors[1])
assert re.match('^[0-9]{4}', errors[2])


def test_bullet_list():
# It fails if no items are supplied:
with pytest.raises(ValueError, match='^This function does'):
bullet_list([], 'Hello', '')
# It uses an alternative singular header:
assert bullet_list(['foo'], 'Hello', 'Bon Jour: {}') == 'Bon Jour: foo'
# It lists items:
assert bullet_list(['foo', 'quartet'], 'Hello:', '') == (
'Hello:'
'\n * foo'
'\n * quartet'
)
Loading