diff --git a/CHANGES.md b/CHANGES.md index 4d2ad34a88d..0305e349bc5 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -44,10 +44,10 @@ default job runner directives for platforms. [#4887](https://github.com/cylc/cylc-flow/pull/4887) - Disallow relative paths in `global.cylc[install]source dirs`. -### Fixes +[#4900](https://github.com/cylc/cylc-flow/pull/4900) - Added a command to assist +with upgrading Cylc 7 workflows to Cylc 8: Try `cylc 728 `. -[#4926](https://github.com/cylc/cylc-flow/pull/4926) - Fix a docstring -formatting problem presenting in the UI mutation flow argument info. +### Fixes [#4891](https://github.com/cylc/cylc-flow/pull/4891) - Fix bug that could cause past jobs to be omitted in the UI. diff --git a/cylc/flow/id.py b/cylc/flow/id.py index 205a79f61c2..ad039e138f5 100644 --- a/cylc/flow/id.py +++ b/cylc/flow/id.py @@ -22,7 +22,6 @@ from enum import Enum import re from typing import ( - Iterable, List, Optional, Tuple, @@ -782,19 +781,13 @@ def detokenise( return '/'.join(identifier) -def upgrade_legacy_ids(*ids: str, relative=False) -> List[str]: +def upgrade_legacy_ids(*ids: str) -> List[str]: """Reformat IDs from legacy to contemporary format: If no upgrading is required it returns the identifiers unchanged. Args: - *ids: - Identifier list. - relative: - If `False` then `ids` must describe absolute ID(s) e.g: - workflow task1.cycle1 task2.cycle2 - If `True` then `ids` should be relative e.g: - task1.cycle1 task2.cycle2 + *ids (tuple): Identifier list. Returns: tuple/list - Identifier list. @@ -822,27 +815,13 @@ def upgrade_legacy_ids(*ids: str, relative=False) -> List[str]: >>> upgrade_legacy_ids('workflow', 'task.123:abc', '234/task:def') ['workflow', '//123/task:abc', '//234/task:def'] - # upgrade relative IDs: - >>> upgrade_legacy_ids('x.1', relative=True) - ['1/x'] - >>> upgrade_legacy_ids('x.1', 'x.2', 'x.3:s', relative=True) - ['1/x', '2/x', '3/x:s'] - """ - if not relative and len(ids) < 2: + if len(ids) < 2: # only legacy relative references require upgrade => abort return list(ids) - legacy_ids: List[str] - _ids: Iterable[str] - if relative: - legacy_ids = [] - _ids = ids - else: - legacy_ids = [ids[0]] - _ids = ids[1:] - - for id_ in _ids: + legacy_ids = [ids[0]] + for id_ in ids[1:]: try: tokens = legacy_tokenise(id_) except ValueError: @@ -851,7 +830,7 @@ def upgrade_legacy_ids(*ids: str, relative=False) -> List[str]: else: # upgrade this token legacy_ids.append( - detokenise(tokens, selectors=True, relative=relative) + detokenise(tokens, selectors=True) ) LOG.warning( diff --git a/cylc/flow/id_cli.py b/cylc/flow/id_cli.py index 76b35cc8ec7..55d9f14a9ee 100644 --- a/cylc/flow/id_cli.py +++ b/cylc/flow/id_cli.py @@ -14,13 +14,13 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . + import asyncio import fnmatch from pathlib import Path import re from typing import Optional, Dict, List, Tuple, Any -from cylc.flow import LOG from cylc.flow.exceptions import ( InputError, WorkflowFilesError, @@ -397,16 +397,6 @@ def _validate_workflow_ids(*tokens_list, src_path): raise InputError( f'Workflow ID cannot be a file: {tokens["workflow"]}' ) - if tokens['cycle'] and tokens['cycle'].startswith('run'): - # issue a warning if the run number is provided after the // - # separator e.g. workflow//run1 rather than workflow/run1// - suggested = Tokens( - user=tokens['user'], - workflow=f'{tokens["workflow"]}/{tokens["cycle"]}', - cycle=tokens['task'], - task=tokens['job'], - ) - LOG.warning(f'Did you mean: {suggested.id}') detect_both_flow_and_suite(src_path) diff --git a/cylc/flow/network/schema.py b/cylc/flow/network/schema.py index 22bfe463800..0f76334e07e 100644 --- a/cylc/flow/network/schema.py +++ b/cylc/flow/network/schema.py @@ -1391,9 +1391,15 @@ def description(self): class Flow(String): - __doc__ = ( - f"""An integer or one of {FLOW_ALL}, {FLOW_NEW} or {FLOW_NONE}.""" - ) + """An integer or one of {FLOW_ALL}, {FLOW_NEW} or {FLOW_NONE}.""" + + +# NOTE: docstrings can't be f-strings so we must manually format it. +Flow.__doc__ = Flow.__doc__.format( # type: ignore[union-attr] + FLOW_ALL=FLOW_ALL, + FLOW_NEW=FLOW_NEW, + FLOW_NONE=FLOW_NONE, +) # Mutations: diff --git a/cylc/flow/option_parsers.py b/cylc/flow/option_parsers.py index b1ce350cd9d..579407a4648 100644 --- a/cylc/flow/option_parsers.py +++ b/cylc/flow/option_parsers.py @@ -77,15 +77,6 @@ def format_shell_examples(string): ) -def verbosity_to_log_level(verb: int) -> int: - """Convert Cylc verbosity to log severity level.""" - if verb < 0: - return logging.WARNING - if verb > 0: - return logging.DEBUG - return logging.INFO - - def verbosity_to_opts(verb: int) -> List[str]: """Convert Cylc verbosity to the CLI opts required to replicate it. @@ -476,7 +467,12 @@ def parse_args(self, api_args, remove_opts=None): # better choice for the logging stream. This allows us to use STDOUT # for verbosity agnostic outputs. # 2. Scheduler will remove this handler when it becomes a daemon. - LOG.setLevel(verbosity_to_log_level(options.verbosity)) + if options.verbosity < 0: + LOG.setLevel(logging.WARNING) + elif options.verbosity > 0: + LOG.setLevel(logging.DEBUG) + else: + LOG.setLevel(logging.INFO) RSYNC_LOG.setLevel(logging.INFO) # Remove NullHandler before add the StreamHandler for log in (LOG, RSYNC_LOG): @@ -534,9 +530,9 @@ class Options: But you can't create new options at initiation, this gives us basic input validation: - >>> PythonOptions(e=6) + >>> opts(e=6) Traceback (most recent call last): - ValueError: e + TypeError: 'Values' object is not callable You can reuse the object multiple times >>> opts2 = PythonOptions(a=2) @@ -550,8 +546,6 @@ def __init__( ) -> None: if overrides is None: overrides = {} - if isinstance(parser, CylcOptionParser) and parser.auto_add: - parser.add_std_options() self.defaults = {**parser.defaults, **overrides} def __call__(self, **kwargs) -> Values: diff --git a/cylc/flow/scheduler_cli.py b/cylc/flow/scheduler_cli.py index e01e203de12..67288e994ad 100644 --- a/cylc/flow/scheduler_cli.py +++ b/cylc/flow/scheduler_cli.py @@ -25,7 +25,6 @@ from cylc.flow import LOG, RSYNC_LOG from cylc.flow.exceptions import ServiceFileError import cylc.flow.flags -from cylc.flow.id import upgrade_legacy_ids from cylc.flow.host_select import select_workflow_host from cylc.flow.hostuserutil import is_remote_host from cylc.flow.id_cli import parse_ids @@ -412,9 +411,4 @@ async def _run(scheduler: Scheduler) -> int: @cli_function(get_option_parser) def play(parser: COP, options: 'Values', id_: str): """Implement cylc play.""" - if options.starttask: - options.starttask = upgrade_legacy_ids( - *options.starttask, - relative=True, - ) return scheduler_cli(options, id_) diff --git a/cylc/flow/scripts/lint.py b/cylc/flow/scripts/lint.py new file mode 100755 index 00000000000..b1cb9a1a429 --- /dev/null +++ b/cylc/flow/scripts/lint.py @@ -0,0 +1,575 @@ +#!/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 . +"""Cylc 728 looks through one or more folders for ``suite*.rc`` files and +search for Cylc 7 syntax which may be problematic at Cylc 8. + +Can be run either as a linter or "in place" (``-i``), leaving comments +in files. If used in the "in place" mode it is recommended that you ensure +that you have recorded the state of your workflow in a version control +system before starting. + +.. warning:: + + When run with ``-i`` (``--inplace``) mode ``Cylc 728`` changes your files. + We strongly recommend committing your workflow to version control + before using ``Cylc 728 -i``. + +Usage +^^^^^ + +.. code-block:: bash + + # run as a linter + cylc 728 + + # run inplace + cylc 728 --inplace + cylc 728 -i + + # Get information about errors: + cylc 728 --reference + cylc 728 -r + +Change Codes +^^^^^^^^^^^^ + +""" +from colorama import Fore +from optparse import Values +from pathlib import Path +import re +from typing import Generator + +from cylc.flow import LOG +from cylc.flow.option_parsers import ( + CylcOptionParser as COP, +) +from cylc.flow.terminal import cli_function + +STYLE_GUIDE = ( + 'https://cylc.github.io/cylc-doc/latest/html/workflow-design-guide/' + 'style-guide.html#' +) +URL_STUB = "https://cylc.github.io/cylc-doc/latest/html/7-to-8/" +SECTION1 = r'\[\s*{}\s*\]' +SECTION2 = r'\[\[\s*{}\s*\]\]' +SECTION3 = r'\[\[\[\s*{}\s*\]\]\]' +FILEGLOBS = ['*.rc', '*.cylc'] +JINJA2_SHEBANG = '#!jinja2' +JINJA2_FOUND_WITHOUT_SHEBANG = 'jinja2 found: no shebang (#!jinja2)' +CHECKS_DESC = {'U': '7 to 8 upgrades', 'S': 'Style'} +CHECKS = { + 'U': { + re.compile(SECTION1.format('visualization')): { + 'short': 'section ``[visualization]`` has been removed.', + 'url': 'summary.html#new-web-and-terminal-uis' + }, + re.compile(SECTION1.format('cylc')): { + 'short': 'section ``[cylc]`` is now called ``[scheduler]``.', + 'url': 'summary.html#terminology' + }, + re.compile(SECTION2.format('authentication')): { + 'short': '``[cylc][authentication]`` is now obsolete.', + 'url': '' + }, + re.compile(r'^\s*include at start-up\s*='): { + 'short': '``[cylc]include at start up`` is obsolete.', + 'url': ( + 'major-changes/excluding-tasks.html?' + '#excluding-tasks-at-start-up-is-not-supported' + ), + }, + re.compile(r'exclude at start-up\s*?='): { + 'short': '``[cylc]exclude at start up`` is obsolete.', + 'url': ( + 'major-changes/excluding-tasks.html?' + '#excluding-tasks-at-start-up-is-not-supported' + ), + }, + re.compile(r'log resolved dependencies\s*?='): { + # Mainly for testing + 'short': '``[cylc]log resolved dependencies`` is obsolete.', + 'url': '' + }, + re.compile(r'required run mode\s*?='): { + # Mainly for testing + 'short': '``[cylc]required run mode`` is obsolete.', + 'url': '' + }, + re.compile(r'health check interval\s*?='): { + 'short': '``[cylc]health check interval`` is obsolete.', + 'url': '' + }, + re.compile(r'abort if any task fails\s*?='): { + 'short': '``[cylc]abort if any task fails`` is obsolete.', + 'url': '' + }, + re.compile(r'disable automatic shutdown\s*?='): { + 'short': '``[cylc]disable automatic shutdown`` is obsolete.', + 'url': '' + }, + re.compile(r'reference test\s*?='): { + # Mainly for testing + 'short': '``[cylc]reference test`` is obsolete.', + 'url': '' + }, + re.compile(r'disable suite event handlers\s*?='): { + 'short': '``[cylc]disable suite event handlers`` is obsolete.', + 'url': '' + }, + re.compile(SECTION2.format('simulation')): { + 'short': '``[cylc]simulation`` is obsolete.', + 'url': '' + }, + re.compile(r'spawn to max active cycle points\s*?='): { + 'short': '``[cylc]spawn to max active cycle points`` is obsolete.', + 'url': ( + 'https://cylc.github.io/cylc-doc/latest/html/reference' + '/config/workflow.html#flow.cylc[scheduling]runahead%20limit' + ), + }, + re.compile(r'abort on stalled\s*?='): { + 'short': + '``[cylc][events]abort on stalled`` is obsolete.', + 'url': '' + }, + re.compile(r'abort if .* handler fails\s*?='): { + 'short': ( + '``[cylc][events]abort if ___ handler fails`` commands are' + ' obsolete.' + ), + 'url': '' + }, + re.compile(r'mail to\s*='): { + 'short': ( + '``[events]mail to`` => ``[mail]to``' + ), + 'url': '' + }, + re.compile(r'mail from\s*='): { + 'short': ( + '``[events]mail from`` => ``[mail]from``' + ), + 'url': '' + }, + re.compile(r'mail footer\s*='): { + 'short': ( + '``[events]mail footer`` => ``[mail]footer``' + ), + 'url': '' + }, + re.compile(r'mail smtp\s*='): { + 'short': ( + '``[events]mail smtp`` => ``global.cylc[scheduler][mail]smtp``' + ), + 'url': '' + }, + re.compile(r'^\s*timeout\s*='): { + 'short': ( + '``[cylc][events]timeout`` => ' + '``[scheduler][events]stall timeout``' + ), + 'url': '' + }, + re.compile(r'^\s*inactivity\s*='): { + 'short': ( + '``[cylc][events]inactivity`` => ' + '``[scheduler][events]inactivity timeout``' + ), + 'url': '' + }, + re.compile(r'abort on inactivity\s*='): { + 'short': ( + '``[cylc][events]abort on inactivity`` => ' + '``[scheduler][events]abort on inactivity timeout``' + ), + 'url': '' + }, + re.compile(r'force run mode\s*='): { + 'short': ( + '``[cylc]force run mode`` is obsolete.' + ), + 'url': '' + }, + re.compile(SECTION2.format('environment')): { + 'short': ( + '``[cylc][environment]`` is obsolete.' + ), + 'url': '' + }, + re.compile(r'.* handler\s*?='): { + 'short': ( + '``[cylc][][events]___ handler`` commands are' + ' now "handlers".' + ), + 'url': '' + }, + re.compile(r'mail retry delays\s*?='): { + 'short': ( + '``[runtime][][events]mail retry delays`` ' + 'is obsolete.' + ), + 'url': '' + }, + re.compile(r'extra log files\s*?='): { + 'short': ( + '``[runtime][][events]extra log files`` ' + 'is obsolete.' + ), + 'url': '' + }, + re.compile(r'shell\s*?='): { + 'short': ( + '``[runtime][]shell`` ' + 'is obsolete.' + ), + 'url': '' + }, + re.compile(r'suite definition directory\s*?='): { + 'short': ( + '``[runtime][][remote]suite definition directory`` ' + 'is obsolete.' + ), + 'url': 'summary.html#symlink-dirs' + }, + re.compile(SECTION2.format('dependencies')): { + 'short': '``[dependencies]`` is deprecated.', + 'url': 'major-changes/config-changes.html#graph' + }, + re.compile(r'graph\s*?='): { + 'short': ( + '``[cycle point]graph =`` is deprecated, ' + 'use ``cycle point = ``' + ), + 'url': 'major-changes/config-changes.html#graph' + }, + re.compile(SECTION2.format('remote')): { + 'short': ( + '``[runtime][][remote]host`` is deprecated, ' + 'use ``[runtime][]platform``' + ), + 'url': 'major-changes/platforms.html#platforms' + }, + re.compile(r'suite state polling\s*='): { + 'short': ( + '``[runtime][]suite state polling`` is deprecated, ' + 'use ``[runtime][]workflow state polling``' + ), + 'url': 'major-changes/platforms.html#platforms' + }, + re.compile(SECTION3.format('job')): { + 'short': ( + '``[runtime][][job]`` is deprecated, ' + 'use ``[runtime][]platform``' + '\n (the following items can be moved to ' + '``[runtime][]``:' + '\n - ``execution retry delays``' + '\n - ``submission retry delays``' + '\n - ``execution polling intervals``' + '\n - ``submission polling intervals``' + '\n - ``execution time limit``' + ), + 'url': 'major-changes/platforms.html#platforms' + }, + re.compile(SECTION2.format('parameter templates')): { + 'short': ( + '``[cylc][parameter templates]`` is deprecated, ' + 'use ``[task parameters][templates]``' + ), + 'url': '' + }, + re.compile(SECTION2.format('parameters')): { + 'short': ( + '``[cylc][parameters]`` is deprecated, ' + 'use ``[task parameters]``' + ), + 'url': '' + }, + re.compile(r'task event mail interval\s*?='): { + 'short': ( + '``[cylc][task event mail interval]`` is deprecated, ' + 'use ``[scheduler][mail][task event batch interval]``' + ), + 'url': '' + }, + re.compile(r'{{.*}}'): { + 'short': ( + f'{JINJA2_FOUND_WITHOUT_SHEBANG}' + '{{VARIABLE}}' + ), + 'url': '' + }, + re.compile(r'{%.*%}'): { + 'short': ( + f'{JINJA2_FOUND_WITHOUT_SHEBANG}' + '{% .* %}' + ), + 'url': '' + }, + re.compile(r'max active cycle points\s*='): { + 'short': ( + '``[scheduling]max active cycle points`` is deprecated' + 'use [scheduling]runahead limit' + ), + 'url': '' + }, + re.compile(r'hold after point\s*='): { + 'short': ( + '``[scheduling]hold after point`` is deprecated' + 'use [scheduling]hold after cycle point' + ), + 'url': '' + }, + }, + 'S': { + re.compile(r'^\t'): { + 'short': 'Use multiple spaces, not tabs', + 'url': STYLE_GUIDE + 'tab-characters' + }, + # Not a full test, but if a non section is not indented... + re.compile(r'^[^\[|\s]'): { + 'short': 'Item not indented.', + 'url': STYLE_GUIDE + 'indentation' + }, + # [section] + re.compile(r'^\s+\[.*\]'): { + 'short': 'Too many indents for top level section.', + 'url': STYLE_GUIDE + 'indentation' + }, + # 2 or 4 space indentation both seem reasonable: + re.compile(r'^(\s|\s{3}|\s{5,})\[\[.*\]\]'): { + 'short': 'wrong number of indents for second level section.', + 'url': STYLE_GUIDE + 'indentation' + }, + re.compile(r'^(\s{1,3}|\s{5,7}|\s{9,})\[\[\[.*\]\]\]'): { + 'short': 'wrong number of indents for third level section.', + 'url': STYLE_GUIDE + 'indentation' + }, + re.compile(r'\s$'): { + 'short': 'trailing whitespace.', + 'url': STYLE_GUIDE + 'trailing-whitespace' + }, + # Consider re-adding this as an option later: + # re.compile(r'^.{80,}'): { + # 'short': 'line > 79 characters.', + # 'url': STYLE_GUIDE + 'line-length-and-continuation' + # }, + re.compile(r'inherit\s*=\s*[a-z].*$'): { + 'short': 'Family name contains lowercase characters.', + 'url': STYLE_GUIDE + 'task-naming-conventions' + }, + } +} + + +def parse_checks(check_arg): + """Collapse metadata in checks dicts. + """ + parsedchecks = {} + if check_arg == '728': + purpose_filters = ['U'] + elif check_arg == 'style': + purpose_filters = ['S'] + else: + purpose_filters = ['U', 'S'] + + for purpose, checks in CHECKS.items(): + if purpose in purpose_filters: + for index, (pattern, meta) in enumerate(checks.items()): + meta.update({'purpose': purpose}) + meta.update({'index': index}) + parsedchecks.update({pattern: meta}) + return parsedchecks + + +def check_cylc_file(file_, checks, modify=False): + """Check A Cylc File for Cylc 7 Config""" + # Set mode as read-write or read only. + outlines = [] + + # Open file, and read it's line to mempory. + lines = file_.read_text().split('\n') + jinja_shebang = lines[0].strip().lower() == JINJA2_SHEBANG + count = 0 + for line_no, line in enumerate(lines, start=1): + for check, message in checks.items(): + # Tests with for presence of Jinja2 if no shebang line is + # present. + if ( + jinja_shebang + and message['short'].startswith( + JINJA2_FOUND_WITHOUT_SHEBANG) + ): + continue + if check.findall(line) and not line.strip().startswith('#'): + count += 1 + if modify: + if message['url'].startswith('http'): + url = message['url'] + else: + url = URL_STUB + message['url'] + outlines.append( + f'# [{message["purpose"]}{message["index"]:03d}]: ' + f'{message["short"]}\n' + f'# - see {url}' + ) + else: + print( + Fore.YELLOW + + f'[{message["purpose"]}{message["index"]:03d}]' + f'{file_}: {line_no}: {message["short"]}' + ) + if modify: + outlines.append(line) + if modify: + file_.write_text('\n'.join(outlines)) + return count + + +def get_cylc_files(base: Path) -> Generator[Path, None, None]: + """Given a directory yield paths to check. + """ + excludes = [Path('log')] + + for rglob in FILEGLOBS: + for path in base.rglob(rglob): + # Exclude log directory: + if path.relative_to(base).parents[0] not in excludes: + yield path + + +def get_reference(checks): + """Print a reference for checks to be carried out. + + Returns: + RST compatible text. + """ + output = '' + current_checkset = '' + for check, meta in checks.items(): + # Check if the purpose has changed - if so create a new + # section title: + if meta['purpose'] != current_checkset: + current_checkset = meta['purpose'] + title = CHECKS_DESC[meta["purpose"]] + output += f'\n{title}\n{"-" * len(title)}\n\n' + + # Fill a template with info about the issue. + template = ( + '{checkset}{index:003d} ``{title}``:\n {summary}\n' + ' see - {url}\n' + ) + if meta['url'].startswith('http'): + url = meta['url'] + else: + url = URL_STUB + meta['url'] + msg = template.format( + title=check.pattern.replace('\\', ''), + checkset=meta['purpose'], + summary=meta['short'], + url=url, + index=meta['index'], + ) + output += msg + output += '\n' + return output + + +def get_option_parser() -> COP: + parser = COP( + __doc__, + argdoc=[('[targets ...]', 'Directories to lint')], + ) + parser.add_option( + '--inplace', '-i', + help=( + 'Modify files in place, adding comments to files. ' + 'If not set, the script will work as a linter' + ), + action='store_true', + default=False, + ) + parser.add_option( + '--ruleset', '-r', + help=( + 'Set of rules to use: ' + '("728", "style", "all")' + ), + default='728', + choices=('728', 'style', 'all'), + dest='linter' + ) + + return parser + + +@cli_function(get_option_parser) +def main(parser: COP, options: 'Values', *targets) -> None: + + # Expand paths so that message will return full path + # & ensure that CWD is used if no path is given: + if targets: + targets = tuple(Path(path).resolve() for path in targets) + else: + targets = (str(Path.cwd()),) + + # Get a list of checks bas ed on the checking options: + count = 0 + # Allow us to check any number of folders at once + for target in targets: + target = Path(target) + if not target.exists(): + LOG.warn(f'Path {target} does not exist.') + else: + # Check whether target is an upgraded Cylc 8 workflow. + # If it isn't then we shouldn't run the 7-to-8 checks upon + # it: + cylc8 = (target / 'flow.cylc').exists() + if not cylc8 and options.linter == '728': + LOG.error( + f'{target} not a Cylc 8 workflow: ' + 'No checks will be made.' + ) + continue + elif not cylc8 and options.linter == 'all': + LOG.error( + f'{target} not a Cylc 8 workflow: ' + 'Checking only for style.' + ) + check_names = parse_checks('style') + else: + check_names = options.linter + + # Check each file: + checks = parse_checks(check_names) + for file_ in get_cylc_files(target): + LOG.debug(f'Checking {file_}') + count += check_cylc_file(file_, checks, options.inplace) + + # Summing up: + if count > 0: + color = Fore.YELLOW + else: + color = Fore.GREEN + msg = ( + f'Checked {target} against {check_names} ' + f'rules and found {count} issues.' + ) + print(f'{color}{"-" * len(msg)}\n{msg}') + + +__doc__ += get_reference(parse_checks('all')) diff --git a/setup.cfg b/setup.cfg index 89b700bfdf9..86660676421 100644 --- a/setup.cfg +++ b/setup.cfg @@ -170,6 +170,7 @@ cylc.command = jobs-poll = cylc.flow.scripts.jobs_poll:main jobs-submit = cylc.flow.scripts.jobs_submit:main kill = cylc.flow.scripts.kill:main + lint = cylc.flow.scripts.lint:main list = cylc.flow.scripts.list:main message = cylc.flow.scripts.message:main pause = cylc.flow.scripts.pause:main diff --git a/tests/functional/cylc-play/10-start-task-upgrade.t b/tests/functional/cylc-play/10-start-task-upgrade.t deleted file mode 100644 index e4c393ccc7a..00000000000 --- a/tests/functional/cylc-play/10-start-task-upgrade.t +++ /dev/null @@ -1,31 +0,0 @@ -#!/usr/bin/env bash -# 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 . - -# ensure that legacy task ids are upgraded automatically when specified -# with --start-task - -. "$(dirname "$0")/test_header" - -set_test_number 2 - -run_fail "${TEST_NAME_BASE}" \ - cylc play Agrajag --start-task foo.123 --start-task bar.234 - -grep_ok \ - 'Cylc7 format is deprecated, using: 123/foo 234/bar' \ - "${TEST_NAME_BASE}.stderr" - diff --git a/tests/unit/scripts/test_lint.py b/tests/unit/scripts/test_lint.py new file mode 100644 index 00000000000..59f27e1c9ac --- /dev/null +++ b/tests/unit/scripts/test_lint.py @@ -0,0 +1,218 @@ +#!/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 . +"""Tests `cylc lint` CLI Utility. +""" +import difflib +from pathlib import Path +import pytest +import re + +from cylc.flow.scripts.lint import ( + CHECKS, + check_cylc_file, + get_cylc_files, + get_reference, + parse_checks +) + + +TEST_FILE = """ +[visualization] + +[cylc] + include at start-up = foo + exclude at start-up = bar + log resolved dependencies = True + required run mode = False + health check interval = PT10M + abort if any task fails = true + suite definition directory = '/woo' + disable automatic shutdown = false + reference test = true + spawn to max active cycle points = false + [[simulation]] + disable suite event handlers = true + [[authentication]] + [[environment]] + force run mode = dummy + [[events]] + abort on stalled = True + abort if startup handler fails= True # deliberately not added a space. + abort if shutdown handler fails= True + abort if timeout handler fails = True + abort if stalled handler fails = True + abort if inactivity handler fails = False + mail to = eleanor.rigby@beatles.lv + mail from = fr.mckenzie@beatles.lv + mail footer = "Collecting The Rice" + mail smtp = 123.456.789.10 + timeout = 30 + inactivity = 30 + abort on inactivity = 30 + [[parameters]] + [[parameter templates]] + [[mail]] + task event mail interval = PT4M # deliberately added lots of spaces. + +[scheduling] + max active cycle points = 5 + hold after point = 20220101T0000Z + [[dependencies]] + [[[R1]]] + graph = foo + + + +[runtime] + [[MYFAM]] + extra log files = True + {% from 'cylc.flow' import LOG %} + script = {{HELLOWORLD}} + suite state polling = PT1H + [[[remote]]] + host = parasite + suite definition directory = '/home/bar' + [[[job]]] + batch system = slurm + shell = fish + [[[events]]] + mail retry delays = PT30S + warning handler = frr.sh + +""" + + +LINT_TEST_FILE = """ +\t[scheduler] + + [scheduler] + +[[dependencies]] + +[runtime] + [[foo]] + inherit = hello + [[[job]]] +something\t +""" + +LINT_TEST_FILE += ('\nscript = the quick brown fox jumps over the lazy dog ' + 'until it becomes clear that this line is far longer the 79 characters.') + + +@pytest.fixture() +def create_testable_file(monkeypatch, capsys): + def _inner(test_file, checks): + monkeypatch.setattr(Path, 'read_text', lambda _: test_file) + check_cylc_file(Path('x'), parse_checks(checks)) + return capsys.readouterr() + return _inner + + +@pytest.mark.parametrize( + 'number', range(len(CHECKS['U'])) +) +def test_check_cylc_file_7to8(create_testable_file, number, capsys): + try: + result = create_testable_file(TEST_FILE, '728').out + assert f'[U{number:03d}]' in result + except AssertionError: + raise AssertionError( + f'missing error number U{number:03d}' + f'{[*CHECKS["U"].keys()][number]}' + ) + + +def test_check_cylc_file_line_no(create_testable_file, capsys): + """It prints the correct line numbers""" + result = create_testable_file(TEST_FILE, '728').out + assert result.split()[1] == '2:' + + +@pytest.mark.parametrize( + 'number', range(len(CHECKS['S'])) +) +def test_check_cylc_file_lint(create_testable_file, number): + try: + assert f'[S{number:03d}]' in create_testable_file( + LINT_TEST_FILE, 'lint').out + except AssertionError: + raise AssertionError( + f'missing error number S:{number:03d}' + f'{[*CHECKS["S"].keys()][number]}' + ) + + +@pytest.fixture +def create_testable_dir(tmp_path): + test_file = (tmp_path / 'suite.rc') + test_file.write_text(TEST_FILE) + check_cylc_file(test_file, parse_checks('all'), modify=True) + return '\n'.join([*difflib.Differ().compare( + TEST_FILE.split('\n'), test_file.read_text().split('\n') + )]) + + +@pytest.mark.parametrize( + 'number', range(len(CHECKS['U'])) +) +def test_check_cylc_file_inplace(create_testable_dir, number): + try: + assert f'[U{number:03d}]' in create_testable_dir + except AssertionError: + raise AssertionError( + f'missing error number {number:03d}:7-to-8 - ' + f'{[*CHECKS["U"].keys()][number]}' + ) + + +def test_get_cylc_files_get_all_rcs(tmp_path): + """It returns all paths except `log/**`. + """ + expect = [('etc', 'foo.rc'), ('bin', 'foo.rc'), ('an_other', 'foo.rc')] + + # Create a fake run directory, including the log file which should not + # be searched: + dirs = ['etc', 'bin', 'log', 'an_other'] + for path in dirs: + thispath = tmp_path / path + thispath.mkdir() + (thispath / 'foo.rc').touch() + + # Run the test + result = [(i.parent.name, i.name) for i in get_cylc_files(tmp_path)] + assert result.sort() == expect.sort() + + +def test_get_reference(): + """It produces a reference file for our linting.""" + ref = get_reference({ + re.compile('not a regex'): { + 'short': 'section `[vizualization]` has been removed.', + 'url': 'some url or other', + 'purpose': 'U', + 'index': 42 + }, + }) + expect = ( + '\n7 to 8 upgrades\n---------------\n\n' + 'U042 ``not a regex``:\n section `[vizualization]` has been ' + 'removed.\n see -' + ' https://cylc.github.io/cylc-doc/latest/html/7-to-8/some url' + ' or other\n\n' + ) + assert ref == expect diff --git a/tests/unit/test_id_cli.py b/tests/unit/test_id_cli.py index 21d6d115126..ff964c99663 100644 --- a/tests/unit/test_id_cli.py +++ b/tests/unit/test_id_cli.py @@ -14,10 +14,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . -import logging import os from pathlib import Path import pytest +from unittest.mock import patch from cylc.flow.async_util import pipe from cylc.flow.exceptions import InputError, WorkflowFilesError @@ -176,8 +176,7 @@ async def test_parse_ids_mixed_src(ids_in, ids_out, abc_src_dir, mock_exists): """It should parse mixed workflows & tasks from src workflows.""" workflows, _ = await parse_ids_async( - *ids_in, constraint='mixed', src=True - ) + *ids_in, constraint='mixed', src=True) assert { workflow_id: [detokenise(tokens) for tokens in tokens_list] for workflow_id, tokens_list in workflows.items() @@ -315,7 +314,7 @@ def src_dir(tmp_path): other_dir.mkdir() other_file = other_dir / 'nugget' other_file.touch() - + os.chdir(tmp_path) yield src_dir os.chdir(cwd_before) @@ -518,7 +517,7 @@ def test_validate_constraint(): _validate_constraint(Tokens(), constraint='mixed') -def test_validate_workflow_ids_basic(tmp_run_dir): +def test_validate_workflow_ids(tmp_run_dir): _validate_workflow_ids(Tokens('workflow'), src_path='') with pytest.raises(InputError): _validate_workflow_ids(Tokens('~alice/workflow'), src_path='') @@ -530,20 +529,6 @@ def test_validate_workflow_ids_basic(tmp_run_dir): ) -def test_validate_workflow_ids_warning(caplog): - """It should warn when the run number is provided as a cycle point.""" - caplog.set_level(logging.WARN) - _validate_workflow_ids(Tokens('workflow/run1//cycle/task'), src_path='') - assert caplog.messages == [] - - _validate_workflow_ids(Tokens('workflow//run1'), src_path='') - assert caplog.messages == ['Did you mean: workflow/run1'] - - caplog.clear() - _validate_workflow_ids(Tokens('workflow//run1/cycle/task'), src_path='') - assert caplog.messages == ['Did you mean: workflow/run1//cycle/task'] - - def test_validate_number(): _validate_number(Tokens('a'), max_workflows=1) with pytest.raises(InputError): diff --git a/tests/unit/test_option_parsers.py b/tests/unit/test_option_parsers.py index dcd2f6e8977..9a8b91c9f01 100644 --- a/tests/unit/test_option_parsers.py +++ b/tests/unit/test_option_parsers.py @@ -21,7 +21,7 @@ import io from contextlib import redirect_stdout import cylc.flow.flags -from cylc.flow.option_parsers import CylcOptionParser as COP, Options +from cylc.flow.option_parsers import CylcOptionParser as COP USAGE_WITH_COMMENT = "usage \n # comment" @@ -85,11 +85,3 @@ def test_help_nocolor(monkeypatch: pytest.MonkeyPatch, parser: COP): with redirect_stdout(f): parser.print_help() assert (f.getvalue()).startswith("Usage: " + USAGE_WITH_COMMENT) - - -def test_Options_std_opts(): - """Test Python Options API with standard options.""" - parser = COP(USAGE_WITH_COMMENT, auto_add=True) - MyOptions = Options(parser) - MyValues = MyOptions(verbosity=1) - assert MyValues.verbosity == 1