diff --git a/CHANGES.md b/CHANGES.md index d5fe9cfd..27af8c00 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -6,13 +6,19 @@ creating a new release entry be sure to copy & paste the span tag with the updated. Only the first match gets replaced, so it's fine to leave the old ones in. --> -## __cylc-rose-1.3.1 (Upcoming)__ +## __cylc-rose-1.3.1 (Released 2023-10-24)__ ### Fixes [#250](https://github.com/cylc/cylc-rose/pull/250) - Prevent project name being manually set to an empty string. +[#225](https://github.com/cylc/cylc-rose/pull/225) - Prevent totally invalid +CLI --defines with no = sign. + +[#248](https://github.com/cylc/cylc-rose/pull/248) - Make sure that +rose stem sets variables in `[jinja2:suite.rc]` not `[jinja2]`. + ## __cylc-rose-1.3.0 (Released 2023-07-21)__ ### Fixes diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 603761fe..28e627b6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -45,6 +45,7 @@ below. - Bruno Kinoshita - Hilary Oliver - Jonny Williams + - Mark Dawson (All contributors are identifiable with email addresses in the git version diff --git a/cylc/rose/entry_points.py b/cylc/rose/entry_points.py index e0d4b4d0..0517bde4 100644 --- a/cylc/rose/entry_points.py +++ b/cylc/rose/entry_points.py @@ -31,6 +31,7 @@ dump_rose_log, get_rose_vars_from_config_node, identify_templating_section, + invalid_defines_check, rose_config_exists, rose_config_tree_loader, merge_rose_cylc_suite_install_conf, @@ -120,6 +121,10 @@ def get_rose_vars(srcdir=None, opts=None): raise NotARoseSuiteException() return config + # Check for definitely invalid defines + if opts and hasattr(opts, 'defines'): + invalid_defines_check(opts.defines) + # Load the raw config tree config_tree = rose_config_tree_loader(srcdir, opts) deprecation_warnings(config_tree) diff --git a/cylc/rose/stem.py b/cylc/rose/stem.py index 11565cd9..ba585775 100644 --- a/cylc/rose/stem.py +++ b/cylc/rose/stem.py @@ -75,6 +75,7 @@ ) from cylc.rose.entry_points import get_rose_vars +from cylc.rose.utilities import id_templating_section import metomi.rose.config from metomi.rose.fs_util import FileSystemUtil @@ -459,10 +460,10 @@ def process(self): self.opts.project.append(project) if i == 0: - # Get the name of the template section to be used: template_type = get_rose_vars( Path(url) / "rose-stem")["templating_detected"] - self.template_section = f'[{template_type}]' + self.template_section = id_templating_section( + template_type, with_brackets=True) # Versions of variables with hostname prepended for working copies url_host = self._prepend_localhost(url) diff --git a/cylc/rose/utilities.py b/cylc/rose/utilities.py index 0295c80c..c570d4ed 100644 --- a/cylc/rose/utilities.py +++ b/cylc/rose/utilities.py @@ -21,15 +21,16 @@ from pathlib import Path import re import shlex -from typing import TYPE_CHECKING, Any, List, Tuple, Union +from typing import TYPE_CHECKING, Any, List, Optional, Tuple, Union from cylc.flow.hostuserutil import get_host from cylc.flow import LOG +from cylc.flow.exceptions import CylcError from cylc.flow.flags import cylc7_back_compat from cylc.rose.jinja2_parser import Parser, patch_jinja2_leading_zeros from metomi.rose import __version__ as ROSE_VERSION from metomi.isodatetime.datetimeoper import DateTimeOperator -from metomi.rose.config import ConfigDumper, ConfigNodeDiff, ConfigNode +from metomi.rose.config import ConfigNodeDiff, ConfigNode, ConfigDumper from metomi.rose.config_processor import ConfigProcessError from metomi.rose.env import env_var_process, UnboundEnvironmentVariableError @@ -46,7 +47,11 @@ ALL_MODES = 'all modes' -class MultipleTemplatingEnginesError(Exception): +class MultipleTemplatingEnginesError(CylcError): + ... + + +class InvalidDefineError(CylcError): ... @@ -179,13 +184,27 @@ def identify_templating_section(config_node): "You should not define more than one templating section. " f"You defined:\n\t{'; '.join(defined_sections)}" ) - elif 'jinja2:suite.rc' in defined_sections: + elif defined_sections: + return id_templating_section(defined_sections.pop()) + else: + return id_templating_section('') + + +def id_templating_section( + section: Optional[str] = None, + with_brackets: bool = False +) -> str: + """Return a full template section string.""" + templating = None + if section and 'jinja2' in section: templating = 'jinja2:suite.rc' - elif 'empy:suite.rc' in defined_sections: + elif section and 'empy' in section: templating = 'empy:suite.rc' - else: + + if not templating: templating = 'template variables' + templating = f'[{templating}]' if with_brackets else templating return templating @@ -314,11 +333,42 @@ def merge_rose_cylc_suite_install_conf(old, new): return old +def invalid_defines_check(defines: List) -> None: + """Check for defines which do not contain an = and therefore cannot be + valid + + Examples: + + # A single invalid define: + >>> import pytest + >>> with pytest.raises(InvalidDefineError, match=r'\\* foo'): + ... invalid_defines_check(['foo']) + + # Two invalid defines and one valid one: + >>> with pytest.raises( + ... InvalidDefineError, match=r'\\* foo.*\\n.* \\* bar' + ... ): + ... invalid_defines_check(['foo', 'bar52', 'baz=442']) + + # No invalid defines + >>> invalid_defines_check(['foo=12']) + """ + invalid_defines = [] + for define in defines: + if parse_cli_defines(define) is False: + invalid_defines.append(define) + if invalid_defines: + msg = 'Invalid Suite Defines (should contain an =)' + for define in invalid_defines: + msg += f'\n * {define}' + raise InvalidDefineError(msg) + + def parse_cli_defines(define: str) -> Union[ - bool, Tuple[ + bool, str, Tuple[ List[Union[str, Any]], Union[str, Any], - Union[str, Any] + Union[str, Any], ] ]: """Parse a define string. diff --git a/mypy.ini b/mypy.ini index 51e43ae4..2097c6f1 100644 --- a/mypy.ini +++ b/mypy.ini @@ -11,3 +11,7 @@ explicit_package_bases = True allow_redefinition = True strict_equality = True show_error_codes = True + +# Suppress the following messages: +# By default the bodies of untyped functions are not checked, consider using --check-untyped-defs +disable_error_code = annotation-unchecked \ No newline at end of file diff --git a/setup.cfg b/setup.cfg index 3b31cc5a..3fa012bd 100644 --- a/setup.cfg +++ b/setup.cfg @@ -75,6 +75,7 @@ tests = flake8-debugger>=4.0.0 flake8-mutable>=1.2.0 flake8-simplify>=0.15.1 + flake8-type-checking; python_version > "3.7" mypy>=0.910 pytest pytest-cov diff --git a/tests/unit/test_config_node.py b/tests/unit/test_config_node.py index 86bf8232..bcd8d422 100644 --- a/tests/unit/test_config_node.py +++ b/tests/unit/test_config_node.py @@ -32,6 +32,7 @@ deprecation_warnings, dump_rose_log, identify_templating_section, + id_templating_section, MultipleTemplatingEnginesError ) @@ -252,6 +253,20 @@ def test_identify_templating_section(node_, expect, raises): identify_templating_section(node) +@pytest.mark.parametrize( + 'input_, expect', + ( + ([None], 'template variables'), + (['jinja2'], 'jinja2:suite.rc'), + (['jinja2:suite.rc'], 'jinja2:suite.rc'), + ([None, True], '[template variables]'), + (['jinja2', True], '[jinja2:suite.rc]'), + ) +) +def test_id_templating_section(input_, expect): + assert id_templating_section(*input_) == expect + + @pytest.fixture def node_with_ROSE_ORIG_HOST(): def _inner(comment=''): diff --git a/tests/unit/test_rose_stem_units.py b/tests/unit/test_rose_stem_units.py index 2c142f64..8a73015b 100644 --- a/tests/unit/test_rose_stem_units.py +++ b/tests/unit/test_rose_stem_units.py @@ -23,11 +23,12 @@ from typing import Any, Tuple from cylc.rose.stem import ( + _get_rose_stem_opts, ProjectNotFoundException, RoseStemVersionException, RoseSuiteConfNotFoundException, StemRunner, - get_source_opt_from_args + get_source_opt_from_args, ) from metomi.rose.reporter import Reporter @@ -411,3 +412,39 @@ def test_ascertain_project_if_name_supplied( ProjectNotFoundException, match='is not a working copy' ): stemrunner._ascertain_project(item) + + +@pytest.mark.parametrize( + 'language, expect', + ( + ('empy', '[empy:suite.rc]'), + ('jinja2', '[jinja2:suite.rc]'), + ('template variables', '[template variables]'), + ) +) +def test_process_template_engine_set_correctly(monkeypatch, language, expect): + """Defines are correctly assigned a [