From 215ede3930860ca096178297dcd3fd0bea7b294a Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Wed, 10 Jul 2024 10:18:55 +0100 Subject: [PATCH] fix.lint.U013 incomplete message Lint: Testing for check function outputting values if check['short'] needs them Apply suggestions from code review --- changes.d/6214.fix.md | 1 + cylc/flow/scripts/lint.py | 44 +++++++---- tests/unit/scripts/test_lint_checkers.py | 95 ++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 15 deletions(-) create mode 100644 changes.d/6214.fix.md create mode 100644 tests/unit/scripts/test_lint_checkers.py diff --git a/changes.d/6214.fix.md b/changes.d/6214.fix.md new file mode 100644 index 0000000000..5c77f3bedc --- /dev/null +++ b/changes.d/6214.fix.md @@ -0,0 +1 @@ +`cylc lint` rules U013 & U015 now tell you which deprecated variables you are using diff --git a/cylc/flow/scripts/lint.py b/cylc/flow/scripts/lint.py index d045ab7083..e9ef1db742 100755 --- a/cylc/flow/scripts/lint.py +++ b/cylc/flow/scripts/lint.py @@ -150,7 +150,7 @@ } -LIST_ITEM = ' * ' +LIST_ITEM = '\n * ' deprecated_string_templates = { @@ -264,7 +264,12 @@ def check_for_suicide_triggers( def check_for_deprecated_environment_variables( line: str ) -> Union[bool, dict]: - """Warn that environment variables with SUITE in are deprecated""" + """Warn that environment variables with SUITE in are deprecated + + Examples: + >>> check_for_deprecated_environment_variables('CYLC_SUITE_HOST') + {'vars': ['CYLC_SUITE_HOST: CYLC_WORKFLOW_HOST']} + """ vars_found = [ f'{k}: {v}' for k, v in DEPRECATED_ENV_VARS.items() if k in line @@ -277,16 +282,21 @@ def check_for_deprecated_environment_variables( return False -def check_for_obsolete_environment_variables(line: str) -> List[str]: +def check_for_obsolete_environment_variables(line: str) -> Dict[str, List]: """Warn that environment variables are obsolete. Examples: >>> this = check_for_obsolete_environment_variables - >>> this('CYLC_SUITE_DEF_PATH') - ['CYLC_SUITE_DEF_PATH'] + >>> this('script = echo $CYLC_SUITE_DEF_PATH') + {'vars': ['CYLC_SUITE_DEF_PATH']} + >>> this('script = echo "irrelevent"') + {} """ - return [i for i in OBSOLETE_ENV_VARS if i in line] + vars_found = [i for i in OBSOLETE_ENV_VARS if i in line] + if vars_found: + return {'vars': vars_found} + return {} def check_for_deprecated_task_event_template_vars( @@ -298,13 +308,10 @@ def check_for_deprecated_task_event_template_vars( >>> this = check_for_deprecated_task_event_template_vars >>> this('hello = "My name is %(suite)s"') - {'list': ' * %(suite)s ⇒ %(workflow)s'} + {'suggest': '\\n * %(suite)s ⇒ %(workflow)s'} - >>> expect = {'list': ( - ... ' * %(suite)s ⇒ %(workflow)s * %(task_url)s' - ... ' - get ``URL`` (if set in :cylc:conf:`[meta]URL`)')} - >>> this('hello = "My name is %(suite)s, %(task_url)s"') == expect - True + >>> this('hello = "My name is %(suite)s, %(task_url)s"') + {'suggest': '\\n * %(suite)s ⇒ %(workf...cylc:conf:`[meta]URL`)'} """ result = [] for key, (regex, replacement) in deprecated_string_templates.items(): @@ -315,7 +322,7 @@ def check_for_deprecated_task_event_template_vars( result.append(f'%({key})s ⇒ %({replacement})s') if result: - return {'list': LIST_ITEM + LIST_ITEM.join(result)} + return {'suggest': LIST_ITEM + LIST_ITEM.join(result)} return None @@ -370,7 +377,14 @@ def check_indentation(line: str) -> bool: def check_lowercase_family_names(line: str) -> bool: - """Check for lowercase in family names.""" + """Check for lowercase in family names. + + Examples: + >>> check_lowercase_family_names(' inherit = FOO') + False + >>> check_lowercase_family_names(' inherit = foo') + True + """ match = INHERIT_REGEX.match(line) if not match: return False @@ -673,7 +687,7 @@ def list_wrapper(line: str, check: Callable) -> Optional[Dict[str, str]]: }, 'U015': { 'short': ( - 'Deprecated template variables.'), + 'Deprecated template variables: {suggest}'), 'rst': ( 'The following template variables, mostly used in event handlers,' 'are deprecated, and should be replaced:' diff --git a/tests/unit/scripts/test_lint_checkers.py b/tests/unit/scripts/test_lint_checkers.py new file mode 100644 index 0000000000..48b8073396 --- /dev/null +++ b/tests/unit/scripts/test_lint_checkers.py @@ -0,0 +1,95 @@ +#!/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 . +"""Test check functions in the `cylc lint` CLI Utility.""" + +import doctest +import json +import pytest +import re + +from cylc.flow.scripts import lint + +VARS = re.compile(r'\{(.*)\}') + +# Functions in Cylc Lint defined with "check_* +CHECKERS = [ + getattr(lint, i) for i in lint.__dir__() if i.startswith('check_')] +# List of checks defined as checks by Cylc Lint +ALL_CHECKS = [ + *lint.MANUAL_DEPRECATIONS.values(), + *lint.STYLE_CHECKS.values(), +] + +finder = doctest.DocTestFinder() + + +@pytest.mark.parametrize( + 'check', + # Those checks that have custom checker functions + # and a short message with variables to insert: + [ + pytest.param(c, id=c.get('function').__name__) for c in ALL_CHECKS + if c.get('function') in CHECKERS + ] +) +def test_custom_checker_doctests(check): + """All check functions have at least one failure doctest + + By forcing each check function to have valid doctests + for the case that linting has failed we are able to + check that the function outputs the correct information + for formatting the short formats. + """ + doctests = finder.find(check['function'])[0] + + msg = f'{check["function"].__name__}: No failure examples in doctest' + assert any(i.want for i in doctests.examples if i.want), msg + + +@pytest.mark.parametrize( + 'ref, check', + # Those checks that have custom checker functions + # and a short message with variables to insert: + [ + (c.get('function'), c) for c in ALL_CHECKS + if c.get('function') in CHECKERS + and VARS.findall(c['short']) + ] +) +def test_custom_checkers_short_formatters(ref, check): + """If a check message has a format string assert that the checker + function will return a dict to be used in + ``check['short'].format(**kwargs)``, based on doctest output. + + ref is useful to allow us to identify the check, even + though not used in the test. + """ + doctests = finder.find(check['function'])[0] + + # Filter doctest examples for cases where there is a json parsable + # want. + examples = [ + eg for eg in [ + json.loads(e.want.replace("'", '"')) + for e in doctests.examples if e.want + ] + if eg + ] + + # Formatting using the example output changes the check short text: + for example in examples: + assert check['short'].format(**example) != check['short']