diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 12215c1f..fafc7435 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -2,7 +2,7 @@ name: build on: release: - types: [ released ] + types: [released] pull_request: workflow_dispatch: diff --git a/.github/workflows/changelog.yml b/.github/workflows/changelog.yml index cadc14fb..5827b36c 100644 --- a/.github/workflows/changelog.yml +++ b/.github/workflows/changelog.yml @@ -12,8 +12,8 @@ jobs: check: runs-on: ubuntu-latest steps: - - uses: scientific-python/action-check-changelogfile@0.2 - env: - CHANGELOG_FILENAME: CHANGES.rst - CHECK_MILESTONE: false - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - uses: scientific-python/action-check-changelogfile@0.2 + env: + CHANGELOG_FILENAME: CHANGES.rst + CHECK_MILESTONE: false + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e5613384..c7fb3ccd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,12 +5,12 @@ on: branches: - master tags: - - '*' + - "*" pull_request: schedule: # Weekly Monday 9AM build # * is a special character in YAML so you have to quote this string - - cron: '0 9 * * 1' + - cron: "0 9 * * 1" workflow_dispatch: concurrency: diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 73c88920..114cd7c4 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,70 +1,58 @@ repos: - -- repo: https://github.com/pre-commit/pre-commit-hooks - rev: v4.5.0 - hooks: - - id: check-added-large-files - - id: check-ast - - id: check-case-conflict - - id: check-yaml - args: ["--unsafe"] - - id: check-toml - - id: check-merge-conflict - - id: check-symlinks - - id: debug-statements - exclude: "src/stpipe/cmdline.py" - - id: detect-private-key - - id: end-of-file-fixer - - id: trailing-whitespace - -- repo: https://github.com/pre-commit/pygrep-hooks - rev: v1.10.0 - hooks: - - id: python-check-blanket-noqa - - id: python-check-mock-methods - - id: rst-directive-colons - - id: rst-inline-touching-normal - - id: text-unicode-replacement-char - -- repo: https://github.com/codespell-project/codespell - rev: v2.2.6 - hooks: - - id: codespell - args: ["--write-changes"] - additional_dependencies: - - tomli - -- repo: https://github.com/ikamensh/flynt/ - rev: '1.0.1' - hooks: - - id: flynt - exclude: "src/stpipe/extern/.*" - -- repo: https://github.com/asottile/pyupgrade - rev: 'v3.15.0' - hooks: - - id: pyupgrade - args: ["--py38-plus"] - -- repo: https://github.com/astral-sh/ruff-pre-commit - rev: 'v0.1.4' - hooks: - - id: ruff - args: ["--fix"] - exclude: "scripts/strun" - -- repo: https://github.com/pycqa/isort - rev: 5.12.0 - hooks: - - id: isort - -- repo: https://github.com/psf/black - rev: 23.10.1 - hooks: - - id: black - -- repo: https://github.com/PyCQA/bandit - rev: 1.7.5 - hooks: - - id: bandit - args: ["-r", "-ll"] + - repo: https://github.com/pre-commit/pre-commit-hooks + rev: v4.5.0 + hooks: + - id: check-added-large-files + - id: check-ast + - id: check-case-conflict + - id: check-yaml + args: ["--unsafe"] + - id: check-toml + - id: check-merge-conflict + - id: check-symlinks + - id: debug-statements + exclude: "src/stpipe/cmdline.py" + - id: detect-private-key + - id: end-of-file-fixer + - id: trailing-whitespace + + - repo: https://github.com/pre-commit/pygrep-hooks + rev: v1.10.0 + hooks: + - id: rst-directive-colons + - id: rst-inline-touching-normal + - id: text-unicode-replacement-char + + - repo: https://github.com/codespell-project/codespell + rev: v2.2.6 + hooks: + - id: codespell + args: ["--write-changes"] + additional_dependencies: + - tomli + + - repo: https://github.com/astral-sh/ruff-pre-commit + rev: "v0.1.4" + hooks: + - id: ruff + args: ["--fix", "--show-fixes"] + exclude: "scripts/strun" + - id: ruff-format + + - repo: https://github.com/adamchainz/blacken-docs + rev: 1.16.0 + hooks: + - id: blacken-docs + additional_dependencies: + - black==22.12.0 + + - repo: https://github.com/pre-commit/mirrors-prettier + rev: "v3.0.2" + hooks: + - id: prettier + + - repo: https://github.com/scientific-python/cookie + rev: 2023.10.27 + hooks: + - id: sp-repo-review + additional_dependencies: ["repo-review[cli]"] diff --git a/CHANGES.rst b/CHANGES.rst index b4f934f4..709c2e7c 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,7 +1,7 @@ 0.6.0 (unreleased) ================== -- +- Update style and linting checking [#103] 0.5.1 (2023-10-02) ================== diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md index ddba00df..8d726b0f 100644 --- a/CODE_OF_CONDUCT.md +++ b/CODE_OF_CONDUCT.md @@ -2,7 +2,6 @@ We expect all "spacetelescope" organization projects to adopt a code of conduct that ensures a productive, respectful environment for all open source contributors and participants. We are committed to providing a strong and enforced code of conduct and expect everyone in our community to follow these guidelines when interacting with others in all forums. Our goal is to keep ours a positive, inclusive, successful, and growing community. The community of participants in open source Astronomy projects is made up of members from around the globe with a diverse set of skills, personalities, and experiences. It is through these differences that our community experiences success and continued growth. - As members of the community, - We pledge to treat all people with respect and provide a harassment- and bullying-free environment, regardless of sex, sexual orientation and/or gender identity, disability, physical appearance, body size, race, nationality, ethnicity, and religion. In particular, sexual language and imagery, sexist, racist, or otherwise exclusionary jokes are not appropriate. diff --git a/pyproject.toml b/pyproject.toml index 1dbbdda8..795cedbd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -48,7 +48,6 @@ strun = 'stpipe.cli.strun:main' requires = [ 'setuptools >=61', 'setuptools_scm[toml] >=3.4', - 'wheel', ] build-backend = 'setuptools.build_meta' @@ -62,11 +61,18 @@ zip-safe = true where = ['src'] [tool.pytest.ini_options] -minversion = 4.6 +minversion = 6 +log_cli_level = "INFO" +xfail_strict = true doctest_plus = true doctest_rst = true text_file_format = 'rst' -addopts = '' +addopts = [ + "--strict-config", + "--strict-markers", + "-ra", + "--color=yes" +] norecursedirs = [ 'src/stpipe/extern', ] @@ -76,15 +82,16 @@ testpaths = [ filterwarnings = [ "error::ResourceWarning", ] +markers = [ + "soctests", +] [tool.ruff] -select = [ - 'E402', # module level import not at top of file - 'E501', # line too long - 'E711', # comparison to None should be ‘if cond is None:’ - 'E722', # do not use bare except, specify exception instead - 'F', # flakes - 'W', # whitespace / deprecation +src = [ + "src", + "tests", + "docs", + "setup.py", ] line-length = 88 extend-exclude = [ @@ -92,15 +99,53 @@ extend-exclude = [ 'src/stpipe/extern', 'scripts/strun', ] -extend-ignore = [ - 'W605', # invalid escape sequence + +[tool.ruff.lint] +extend-select = [ + 'F', # Pyflakes + 'W', 'E', # pycodestyle + 'I', # isort + 'N', # pep8-naming + 'UP', # pyupgrade + 'S', # flake8-bandit + # 'BLE', # flake8-blind-except + 'B', # flake8-bugbear + 'A', # flake8-builtins (prevent shadowing of builtins) + 'C4', # flake8-comprehensions (best practices for comprehensions) + 'T10', # flake8-debugger (prevent debugger statements in code) + 'ISC', # flake8-implicit-str-concat (prevent implicit string concat) + 'ICN', # flake8-import-conventions (enforce import conventions) + 'INP', # flake8-no-pep420 (prevent use of PEP420, i.e. implicit name spaces) + 'G', # flake8-logging-format (best practices for logging) + 'PIE', # flake8-pie (misc suggested improvement linting) + 'T20', # flake8-print (prevent print statements in code) + 'PT', # flake8-pytest-style (best practices for pytest) + 'Q', # flake8-quotes (best practices for quotes) + 'RSE', # flake8-raise (best practices for raising exceptions) + 'RET', # flake8-return (best practices for return statements) + # 'SLF', # flake8-self (prevent private member access) + 'TID', # flake8-tidy-imports (prevent banned api and best import practices) + 'INT', # flake8-gettext (when to use printf style strings) + 'ARG', # flake8-unused-arguments (prevent unused arguments) + # 'PTH', # flake8-use-pathlib (prefer pathlib over os.path) + 'ERA', # eradicate (remove commented out code) + 'PGH', # pygrep (simple grep checks) + # 'PL', # pylint (general linting, flake8 alternative) + 'FLY', # flynt (f-string conversion where possible) + 'NPY', # NumPy-specific checks (recommendations from NumPy) + 'PERF', # Perflint (performance linting) + 'RUF', # ruff specific checks +] +ignore = [ + "ISC001", # conflicts with ruff formatter ] -[tool.isort] -profile = "black" -filter_files = true -line_length = 88 -extend_skip_glob = ["src/stpipe/extern/*"] +[tool.ruff.lint.extend-per-file-ignores] +"tests/*.py" = ["S101", "S603", "S607", "INP001", "ARG001"] +"src/stpipe/tests/*.py" = ["S101"] +"src/stpipe/cli/*.py" = ["T201"] +"src/stpipe/cmdline.py" = ["T201"] + [tool.black] line-length = 88 @@ -123,3 +168,12 @@ exclude = ["src/stpipe/extern/*"] skip="*.pdf,*.fits,*.asdf,.tox,build,./tags,.git,docs/_build" # ignore-words-list=""" # """ + + +[tool.repo-review] +ignore = [ + "GH200", # Use dependabot + "PC140", # add MyPy to pre-commit + "PC901", # custom pre-comit.ci message + "MY100", # Use MyPy +] diff --git a/src/stpipe/cli/command.py b/src/stpipe/cli/command.py index e78b8477..47de9600 100644 --- a/src/stpipe/cli/command.py +++ b/src/stpipe/cli/command.py @@ -16,7 +16,6 @@ def get_name(cls): ------- str """ - pass @abc.abstractclassmethod def add_subparser(cls, subparsers): @@ -27,7 +26,6 @@ def add_subparser(cls, subparsers): ---------- subparsers : argparse._SubParsersAction """ - pass @abc.abstractclassmethod def run(cls, args): @@ -44,4 +42,3 @@ def run(cls, args): int Exit status code. """ - pass diff --git a/src/stpipe/cli/list.py b/src/stpipe/cli/list.py index 0b229ad7..43616c60 100644 --- a/src/stpipe/cli/list.py +++ b/src/stpipe/cli/list.py @@ -6,7 +6,8 @@ import re import sys -from .. import entry_points +from stpipe import entry_points + from .command import Command diff --git a/src/stpipe/cli/main.py b/src/stpipe/cli/main.py index cccf1b37..290503e0 100644 --- a/src/stpipe/cli/main.py +++ b/src/stpipe/cli/main.py @@ -5,7 +5,8 @@ import sys import traceback -from ..exceptions import StpipeExitException +from stpipe.exceptions import StpipeExitException + from .list import ListCommand # New subclasses of Command must be imported @@ -90,8 +91,7 @@ def _print_versions(): that register an stpipe.steps entry point. """ import stpipe - - from .. import entry_points + from stpipe import entry_points packages = sorted( {(s.package_name, s.package_version) for s in entry_points.get_steps()}, diff --git a/src/stpipe/cmdline.py b/src/stpipe/cmdline.py index 63b17537..ce90af4c 100644 --- a/src/stpipe/cmdline.py +++ b/src/stpipe/cmdline.py @@ -45,10 +45,10 @@ def _get_config_and_class(identifier): step_class = utilities.import_class( utilities.resolve_step_class_alias(identifier), Step ) - except (ImportError, AttributeError, TypeError): + except (ImportError, AttributeError, TypeError) as err: raise ValueError( f"{identifier!r} is not a path to a config file or a Python Step class" - ) + ) from err # Don't validate yet config = config_parser.config_from_dict({}) name = None @@ -82,10 +82,12 @@ def _build_arg_parser_from_spec(spec, step_class, parent=None): description=step_class.__doc__, ) - def build_from_spec(subspec, parts=[]): + def build_from_spec(subspec, parts=None): + if parts is None: + parts = [] for key, val in subspec.items(): if isinstance(val, dict): - build_from_spec(val, parts + [key]) + build_from_spec(val, [*parts, key]) else: comment = subspec.inline_comments.get(key) or "" comment = comment.lstrip("#").strip() @@ -95,14 +97,14 @@ def build_from_spec(subspec, parts=[]): help_string = comment else: help_string = f"{comment} [{default_value_string}]" - argument = "--" + ".".join(parts + [key]) + argument = "--" + ".".join([*parts, key]) if argument[2:] in built_in_configuration_parameters: raise ValueError( "The Step's spec is trying to override a built-in parameter" f" {argument!r}" ) parser.add_argument( - "--" + ".".join(parts + [key]), + "--" + ".".join([*parts, key]), type=str, help=help_string, metavar="", @@ -130,8 +132,6 @@ class FromCommandLine(str): use isinstance to see where the values came from. """ - pass - def _override_config_from_args(config, args): """ @@ -263,7 +263,9 @@ def just_the_step_from_cmdline(args, cls=None): try: log.load_configuration(log_config) except Exception as e: - raise ValueError(f"Error parsing logging config {log_config!r}:\n{e}") + raise ValueError( + f"Error parsing logging config {log_config!r}:\n{e}" + ) from e except Exception as e: _print_important_message("ERROR PARSING CONFIGURATION:", str(e)) parser1.print_help() @@ -336,7 +338,7 @@ def just_the_step_from_cmdline(args, cls=None): # If the configobj validator failed, print usage information. _print_important_message("ERROR PARSING CONFIGURATION:", str(e)) parser2.print_help() - raise ValueError(str(e)) + raise ValueError(str(e)) from e # Define the primary input file. # Always have an output_file set on the outermost step @@ -394,7 +396,7 @@ def step_from_cmdline(args, cls=None): _print_important_message(f"ERROR RUNNING STEP {step_class.__name__!r}:", str(e)) if debug_on_exception: - import pdb + import pdb # noqa: T100 pdb.post_mortem() else: @@ -406,6 +408,7 @@ def step_from_cmdline(args, cls=None): def step_script(cls): import sys - assert issubclass(cls, Step) + if not issubclass(cls, Step): + raise AssertionError("cls must be a subclass of Step") return step_from_cmdline(sys.argv[1:], cls=cls) diff --git a/src/stpipe/config.py b/src/stpipe/config.py index a6fb3932..b7c79a09 100644 --- a/src/stpipe/config.py +++ b/src/stpipe/config.py @@ -149,10 +149,10 @@ def from_asdf(cls, asdf_file): try: _validate_asdf(asdf_file, _LEGACY_CONFIG_SCHEMA_URI) return cls._from_legacy_tree(asdf_file.tree) - except asdf.ValidationError: + except asdf.ValidationError as err: # Raise the original error so that we encourage # use of the new config file format. - raise e + raise e from err @classmethod def _from_tree(cls, tree): diff --git a/src/stpipe/config_parser.py b/src/stpipe/config_parser.py index 09065be1..7981896a 100644 --- a/src/stpipe/config_parser.py +++ b/src/stpipe/config_parser.py @@ -70,33 +70,34 @@ def _output_file_check(path): path = os.path.join(root_dir, path) path = os.path.abspath(path) - dir = os.path.dirname(path) - if dir and not os.path.exists(dir): - os.makedirs(dir) + dir_ = os.path.dirname(path) + if dir_ and not os.path.exists(dir_): + os.makedirs(dir_) return path return _output_file_check -def _is_datamodel(value, default=None): +def _is_datamodel(value): """Verify that value is either is a DataModel.""" if isinstance(value, AbstractDataModel): return value - else: - raise VdtTypeError(value) + + raise VdtTypeError(value) -def _is_string_or_datamodel(value, default=None): +def _is_string_or_datamodel(value): """Verify that value is either a string (nominally a reference file path) or a DataModel (possibly one with no corresponding file.) """ if isinstance(value, AbstractDataModel): return value - elif isinstance(value, str): + + if isinstance(value, str): return value - else: - raise VdtTypeError(value) + + raise VdtTypeError(value) def load_config_file(config_file): @@ -187,7 +188,7 @@ def load_spec_file(cls, preserve_comments=_not_set): """ if preserve_comments is not _not_set: msg = "preserve_comments is deprecated" - warnings.warn(msg, DeprecationWarning) + warnings.warn(msg, DeprecationWarning, stacklevel=2) # Don't use 'hasattr' here, because we don't want to inherit spec # from the base class. if not isclass(cls): @@ -211,7 +212,7 @@ def load_spec_file(cls, preserve_comments=_not_set): raise_errors=True, list_values=False, ) - return + return None def merge_config(into, new): @@ -355,12 +356,12 @@ def validate( else: section_list.append("[missing section]") section_string = "/".join(section_list) - if err == False: + if err is False: if allow_missing: config[key] = spec[key] continue - else: - err = "missing" + + err = "missing" messages.append(f"Config parameter {section_string!r}: {err}") @@ -392,7 +393,6 @@ def string_to_python_type(section, key): else: typed_val = _parse(val) section[key] = typed_val - return def _parse(val): @@ -401,12 +401,15 @@ def _parse(val): """ if val.lower() == "true": return True - elif val.lower() == "false": + + if val.lower() == "false": return False + try: return int(val) except ValueError: pass + try: return float(val) except ValueError: diff --git a/src/stpipe/crds_client.py b/src/stpipe/crds_client.py index 24b44ad8..2e48b517 100644 --- a/src/stpipe/crds_client.py +++ b/src/stpipe/crds_client.py @@ -49,8 +49,7 @@ def get_multiple_reference_paths(parameters, reference_file_types, observatory): raise TypeError("First argument must be a dict of parameters") log.set_log_time(True) - refpaths = _get_refpaths(parameters, tuple(reference_file_types), observatory) - return refpaths + return _get_refpaths(parameters, tuple(reference_file_types), observatory) def _get_refpaths(data_dict, reference_file_types, observatory): @@ -67,11 +66,10 @@ def _get_refpaths(data_dict, reference_file_types, observatory): reftypes=reference_file_types, observatory=observatory, ) - refpaths = { + return { filetype: filepath if "N/A" not in filepath.upper() else "N/A" for (filetype, filepath) in bestrefs.items() } - return refpaths def check_reference_open(refpath): diff --git a/src/stpipe/datamodel.py b/src/stpipe/datamodel.py index 4ee7259f..7ec90b04 100644 --- a/src/stpipe/datamodel.py +++ b/src/stpipe/datamodel.py @@ -16,16 +16,16 @@ class without requiring that they inherit this class. """ @classmethod - def __subclasshook__(cls, C): + def __subclasshook__(cls, c_): """ Pseudo subclass check based on these attributes and methods """ if cls is AbstractDataModel: - mro = C.__mro__ + mro = c_.__mro__ if ( - any([hasattr(CC, "crds_observatory") for CC in mro]) - and any([hasattr(CC, "get_crds_parameters") for CC in mro]) - and any([hasattr(CC, "save") for CC in mro]) + any(hasattr(CC, "crds_observatory") for CC in mro) + and any(hasattr(CC, "get_crds_parameters") for CC in mro) + and any(hasattr(CC, "save") for CC in mro) ): return True return False @@ -34,7 +34,6 @@ def __subclasshook__(cls, C): @abc.abstractmethod def crds_observatory(self): """This should return a string identifying the observatory as CRDS expects it""" - pass @abc.abstractmethod def get_crds_parameters(self): @@ -65,4 +64,3 @@ def save(self, path, dir_path=None, *args, **kwargs): output_path: str The file path the model was saved in. """ - pass diff --git a/src/stpipe/entry_points.py b/src/stpipe/entry_points.py index 061f67c2..133fa6ac 100644 --- a/src/stpipe/entry_points.py +++ b/src/stpipe/entry_points.py @@ -33,14 +33,17 @@ class alias, and the third is a bool indicating whether the class is to be try: elements = entry_point.load()() + package_steps = [ + StepInfo(*element, package_name, package_version) + for element in elements + ] - for element in elements: - package_steps.append(StepInfo(*element, package_name, package_version)) except Exception as e: warnings.warn( f"{STEPS_GROUP} plugin from package {package_name}=={package_version} " "failed to load:\n\n" - f"{e.__class__.__name__}: {e}" + f"{e.__class__.__name__}: {e}", + stacklevel=2, ) steps.extend(package_steps) diff --git a/src/stpipe/exceptions.py b/src/stpipe/exceptions.py index ba579662..fe342c83 100644 --- a/src/stpipe/exceptions.py +++ b/src/stpipe/exceptions.py @@ -1,10 +1,8 @@ -class StpipeException(Exception): +class StpipeException(Exception): # noqa: N818 """ Base class for exceptions from the stpipe package. """ - pass - class StpipeExitException(StpipeException): """ diff --git a/src/stpipe/format_template.py b/src/stpipe/format_template.py index f2b8eb77..9dac089f 100644 --- a/src/stpipe/format_template.py +++ b/src/stpipe/format_template.py @@ -127,7 +127,7 @@ def __init__(self, separator="_", key_formats=None, remove_unused=False): if key_formats: self.key_formats.update(key_formats) - def format(self, format_string, **kwargs): + def format(self, format_string, **kwargs): # noqa: A003 """Perform the formatting Parameters @@ -154,7 +154,7 @@ def format(self, format_string, **kwargs): # 0: The first replacement field. There should only be one. # 2: Get the format spec. # -1: Get the last character representing the type. - format_type = list(self.parse(key_format))[0][2][-1] + format_type = next(iter(self.parse(key_format)))[2][-1] try: value = key_format.format(CONVERSION[format_type](value)) @@ -164,12 +164,8 @@ def format(self, format_string, **kwargs): break else: raise RuntimeError( - "No suitable formatting for {key}: {value} found. Given" - " formatting options:\n\t{formats}".format( - key=key, - value=value, - formats=self.key_formats[key], - ) + f"No suitable formatting for {key}: {value} found. Given" + f" formatting options:\n\t{self.key_formats[key]}" ) formatted_kwargs[key] = value result = super().format(format_string, **formatted_kwargs) @@ -181,15 +177,13 @@ def format(self, format_string, **kwargs): for unused in unused_keys if formatted_kwargs[unused] is not None ] - result_parts = [result] + unused_values - result = self.separator.join(result_parts) - - return result + result_parts = [result, *unused_values] + return self.separator.join(result_parts) # Make the instance callable __call__ = format - def get_value(self, key, args, kwargs): + def get_value(self, key, args, kwargs): # noqa: ARG002 """Return a given field value Parameters diff --git a/src/stpipe/hooks.py b/src/stpipe/hooks.py index 5f979426..2ab16fc1 100644 --- a/src/stpipe/hooks.py +++ b/src/stpipe/hooks.py @@ -1,31 +1,28 @@ """ Pre- and post-hooks """ +import contextlib import types from . import utilities from .step import Step -def hook_from_string(step, type, num, command): +def hook_from_string(step, type, num, command): # noqa: A002 name = f"{type}_hook{num:d}" step_class = None - try: + with contextlib.suppress(Exception): step_class = utilities.import_class(command, Step, step.config_file) - except Exception: - pass if step_class is not None: return step_class(name, parent=step, config_file=step.config_file) step_func = None - try: + with contextlib.suppress(Exception): step_func = utilities.import_class( command, types.FunctionType, step.config_file ) - except Exception: - pass if step_func is not None: from . import function_wrapper @@ -39,5 +36,5 @@ def hook_from_string(step, type, num, command): return SystemCall(name, parent=step, command=command) -def get_hook_objects(step, type, hooks): - return [hook_from_string(step, type, i, hook) for i, hook in enumerate(hooks)] +def get_hook_objects(step, type_, hooks): + return [hook_from_string(step, type_, i, hook) for i, hook in enumerate(hooks)] diff --git a/src/stpipe/log.py b/src/stpipe/log.py index c0dbbb16..5d0a9fc5 100644 --- a/src/stpipe/log.py +++ b/src/stpipe/log.py @@ -32,7 +32,7 @@ # LOGS AS EXCEPTIONS -class LoggedException(Exception): +class LoggedException(Exception): # noqa: N818 """ This is an exception used when a log record is converted into an exception. @@ -83,7 +83,7 @@ def __init__( handler=None, level=logging.NOTSET, break_level=logging.NOTSET, - format=None, + format=None, # noqa: A002 ): if name in ("", ".", "root"): name = "*" @@ -97,7 +97,7 @@ def __init__( self.level = level self.break_level = break_level if format is None: - format = DEFAULT_FORMAT + format = DEFAULT_FORMAT # noqa: A001 self.format = format def match(self, log_name): @@ -117,14 +117,17 @@ def get_handler(self, handler_str): """ if handler_str.startswith("file:"): return logging.FileHandler(handler_str[5:], "w", "utf-8", True) - elif handler_str.startswith("append:"): + + if handler_str.startswith("append:"): return logging.FileHandler(handler_str[7:], "a", "utf-8", True) - elif handler_str == "stdout": + + if handler_str == "stdout": return logging.StreamHandler(sys.stdout) - elif handler_str == "stderr": + + if handler_str == "stderr": return logging.StreamHandler(sys.stderr) - else: - raise ValueError(f"Can't parse handler {handler_str!r}") + + raise ValueError(f"Can't parse handler {handler_str!r}") def apply(self, log): """ @@ -180,10 +183,11 @@ def _level_check(value): value = int(value) except ValueError: pass + try: value = logging._checkLevel(value) - except ValueError: - raise validate.VdtTypeError(value) + except ValueError as err: + raise validate.VdtTypeError(value) from err return value spec = config_parser.load_spec_file(LogConfig) @@ -203,10 +207,8 @@ def _level_check(value): cfg.match_and_apply(log) -def getLogger(name=None): - log = logging.getLogger(name) - - return log +def getLogger(name=None): # noqa: N802 + return logging.getLogger(name) def _find_logging_config_file(): @@ -217,8 +219,7 @@ def _find_logging_config_file(): if os.path.exists(file): return os.path.abspath(file) - buffer = io.BytesIO(DEFAULT_CONFIGURATION) - return buffer + return io.BytesIO(DEFAULT_CONFIGURATION) ########################################################################### @@ -252,9 +253,11 @@ def log(self): @log.setter def log(self, log): - assert log is None or ( + if log is not None and not ( isinstance(log, logging.Logger) and log.name.startswith(STPIPE_ROOT_LOGGER) - ) + ): + raise AssertionError("Can't set the log to a root logger") + self._logs[threading.current_thread()] = log diff --git a/src/stpipe/pipeline.py b/src/stpipe/pipeline.py index 357cc0a0..c84b3078 100644 --- a/src/stpipe/pipeline.py +++ b/src/stpipe/pipeline.py @@ -3,6 +3,7 @@ """ from collections.abc import Sequence from os.path import dirname, join +from typing import ClassVar from . import config_parser, crds_client, log from .extern.configobj.configobj import ConfigObj, Section @@ -24,7 +25,7 @@ class Pipeline(Step): """ # A set of steps used in the Pipeline. Should be overridden by # the subclass. - step_defs = {} + step_defs: ClassVar = {} def __init__(self, *args, **kwargs): """ @@ -173,7 +174,7 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None) disable = get_disable_crds_steppars() if disable: logger.debug( - f"{reftype.upper()}: CRDS parameter reference retrieval disabled." + "%s: CRDS parameter reference retrieval disabled.", reftype.upper() ) return refcfg @@ -196,7 +197,7 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None) ) # # Now merge any config parameters from the step cfg file - logger.debug(f"Retrieving pipeline {reftype.upper()} parameters from CRDS") + logger.debug("Retrieving pipeline %s parameters from CRDS", reftype.upper()) try: ref_file = crds_client.get_reference_file( crds_parameters, @@ -204,13 +205,13 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None) crds_observatory, ) except (AttributeError, crds_client.CrdsError): - logger.debug(f"{reftype.upper()}: No parameters found") + logger.debug("%s: No parameters found", reftype.upper()) else: if ref_file != "N/A": - logger.info(f"{reftype.upper()} parameters found: {ref_file}") + logger.info("%s parameters found: %s", reftype.upper(), ref_file) refcfg = cls.merge_pipeline_config(refcfg, ref_file) else: - logger.debug(f"No {reftype.upper()} reference files found.") + logger.debug("No %s reference files found.", reftype.upper()) return refcfg @@ -266,7 +267,7 @@ def _precache_references(self, input_file): ) as model: self._precache_references_opened(model) except (ValueError, TypeError, OSError): - self.log.info(f"First argument {input_file} does not appear to be a model") + self.log.info("First argument %s does not appear to be a model", input_file) def _precache_references_opened(self, model_or_container): """Pre-fetches references for `model_or_container`. @@ -308,10 +309,9 @@ def _precache_references_impl(self, model): fetch_types = sorted(set(self.reference_file_types) - set(ovr_refs.keys())) self.log.info( - "Prefetching reference files for dataset: " - + repr(model.meta.filename) - + " reftypes = " - + repr(fetch_types) + "Prefetching reference files for dataset: %r reftypes = %r", + model.meta.filename, + fetch_types, ) crds_refs = crds_client.get_multiple_reference_paths( model.get_crds_parameters(), fetch_types, model.crds_observatory @@ -321,7 +321,9 @@ def _precache_references_impl(self, model): for reftype, refpath in sorted(ref_path_map.items()): how = "Override" if reftype in ovr_refs else "Prefetch" - self.log.info(f"{how} for {reftype.upper()} reference file is '{refpath}'.") + self.log.info( + "%s for %s reference file is '%s'.", how, reftype.upper(), refpath + ) crds_client.check_reference_open(refpath) def get_pars(self, full_spec=True): @@ -344,7 +346,7 @@ def get_pars(self, full_spec=True): """ pars = super().get_pars(full_spec=full_spec) pars["steps"] = {} - for step_name, step_class in self.step_defs.items(): + for step_name in self.step_defs.keys(): pars["steps"][step_name] = getattr(self, step_name).get_pars( full_spec=full_spec ) diff --git a/src/stpipe/step.py b/src/stpipe/step.py index 2145eca2..7ce9f7b3 100644 --- a/src/stpipe/step.py +++ b/src/stpipe/step.py @@ -5,7 +5,7 @@ import os import sys from collections.abc import Sequence -from contextlib import contextmanager +from contextlib import contextmanager, suppress from functools import partial from os.path import ( abspath, @@ -18,6 +18,7 @@ split, splitext, ) +from typing import ClassVar try: from astropy.io import fits @@ -69,7 +70,7 @@ class Step: # Reference types for both command line override # definition and reference prefetch - reference_file_types = [] + reference_file_types: ClassVar = [] # Set to False in subclasses to skip prefetch, # but by default attempt to prefetch @@ -87,7 +88,7 @@ def get_config_reftype(cls): return f"pars-{cls.__name__.lower()}" @classmethod - def merge_config(cls, config, config_file): + def merge_config(cls, config, config_file): # noqa: ARG003 return config @classmethod @@ -187,7 +188,13 @@ def from_cmdline(args): return cmdline.step_from_cmdline(args) @classmethod - def _parse_class_and_name(cls, config, parent=None, name=None, config_file=None): + def _parse_class_and_name( + cls, + config, + parent=None, # noqa: ARG003 + name=None, + config_file=None, + ): if "class" in config: step_class = utilities.import_class( utilities.resolve_step_class_alias(config["class"]), @@ -285,7 +292,7 @@ def from_config_section( else: kwargs[k] = config[k] - step = cls( + return cls( name=name, parent=parent, config_file=config_file, @@ -293,8 +300,6 @@ def from_config_section( **kwargs, ) - return step - def __init__( self, name=None, @@ -345,9 +350,9 @@ def __init__( name = self.__class__.__name__ self.name = name if parent is None: - self.qualified_name = ".".join([log.STPIPE_ROOT_LOGGER, self.name]) + self.qualified_name = f"{log.STPIPE_ROOT_LOGGER}.{self.name}" else: - self.qualified_name = ".".join([parent.qualified_name, self.name]) + self.qualified_name = f"{parent.qualified_name}.{self.name}" self.parent = parent # Set the parameters as member variables @@ -360,7 +365,10 @@ def __init__( self.log.setLevel(log.logging.DEBUG) # Log the fact that we have been init-ed. - self.log.info(f"{self.__class__.__name__} instance created.") + self.log.info( + "%s instance created.", + self.__class__.__name__, + ) # Store the config file path so config filenames can be resolved # against it. @@ -386,7 +394,9 @@ def _check_args(self, args, discouraged_types, msg): for i, arg in enumerate(args): if isinstance(arg, discouraged_types): self.log.error( - f"{msg} {i} object. Use an instance of AbstractDataModel instead." + "%s %s object. Use an instance of AbstractDataModel instead.", + msg, + i, ) @property @@ -417,9 +427,9 @@ def run(self, *args): step_result = None - self.log.info(f"Step {self.name} running with args {args}.") + self.log.info("Step %s running with args %s.", self.name, args) - self.log.info(f"Step {self.name} parameters are: {self.get_pars()}") + self.log.info("Step %s parameters are: %s", self.name, self.get_pars()) if len(args): self.set_primary_input(args[0]) @@ -456,10 +466,11 @@ def run(self, *args): model[ f"meta.cal_step.{self.class_alias}" ] = "SKIPPED" - except AttributeError as e: + except AttributeError as e: # noqa: PERF203 self.log.info( - "Could not record skip into DataModel" - f" header: {e}" + "Could not record skip into DataModel " + "header: %s", + e, ) elif isinstance(args[0], AbstractDataModel): try: @@ -469,7 +480,8 @@ def run(self, *args): except AttributeError as e: self.log.info( "Could not record skip into DataModel" - f" header: {e}" + " header: %s", + e, ) step_result = args[0] else: @@ -479,7 +491,9 @@ def run(self, *args): step_result = self.process(*args) except TypeError as e: if "process() takes exactly" in str(e): - raise TypeError("Incorrect number of arguments to step") + raise TypeError( + "Incorrect number of arguments to step" + ) from e raise # Warn if returning a discouraged object @@ -533,10 +547,10 @@ def run(self, *args): " `--save_results=false`" ) else: - self.log.info(f"Saving file {output_path}") + self.log.info("Saving file %s", output_path) result.save(output_path, overwrite=True) - self.log.info(f"Step {self.name} done") + self.log.info("Step %s done", self.name) finally: log.delegator.log = orig_log @@ -559,8 +573,8 @@ def finalize_result(self, result, reference_files_used): List of reference files used when running the step, each a tuple in the form (str reference type, str reference URI). """ - pass + @staticmethod def remove_suffix(name): """ Remove a known Step filename suffix from a filename @@ -700,16 +714,16 @@ def search_attr(self, attribute, default=None, parent_first=False): if value is None: value = getattr(self, attribute, default) return value - else: - value = getattr(self, attribute, None) - if value is None: - try: - value = self.parent.search_attr(attribute) - except AttributeError: - pass - if value is None: - value = default - return value + + value = getattr(self, attribute, None) + if value is None: + try: + value = self.parent.search_attr(attribute) + except AttributeError: + pass + if value is None: + value = default + return value def _precache_references(self, input_file): """Because Step precaching precedes calls to get_reference_file() almost @@ -719,7 +733,6 @@ def _precache_references(self, input_file): true precache operations and avoids having to override the more complex Step.run() instead. """ - pass def get_ref_override(self, reference_file_type): """Determine and return any override for `reference_file_type`. @@ -732,8 +745,8 @@ def get_ref_override(self, reference_file_type): path = getattr(self, override_name, None) if isinstance(path, AbstractDataModel): return path - else: - return abspath(path) if path and path != "N/A" else path + + return abspath(path) if path and path != "N/A" else path def get_reference_file(self, input_file, reference_file_type): """ @@ -765,7 +778,8 @@ def get_reference_file(self, input_file, reference_file_type): (reference_file_type, override.override_handle) ) return override - elif override.strip() != "": + + if override.strip() != "": self._reference_files_used.append( (reference_file_type, basename(override)) ) @@ -841,12 +855,12 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None) disable = get_disable_crds_steppars() if disable: logger.info( - f"{reftype.upper()}: CRDS parameter reference retrieval disabled." + "%s: CRDS parameter reference retrieval disabled.", reftype.upper() ) return config_parser.ConfigObj() # Retrieve step parameters from CRDS - logger.debug(f"Retrieving step {reftype.upper()} parameters from CRDS") + logger.debug("Retrieving step %s parameters from CRDS", reftype.upper()) try: ref_file = crds_client.get_reference_file( crds_parameters, @@ -854,23 +868,23 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None) crds_observatory, ) except (AttributeError, crds_client.CrdsError): - logger.debug(f"{reftype.upper()}: No parameters found") + logger.debug("%s: No parameters found", reftype.upper()) return config_parser.ConfigObj() if ref_file != "N/A": - logger.info(f"{reftype.upper()} parameters found: {ref_file}") + logger.info("%s parameters found: %s", reftype.upper(), ref_file) ref = config_parser.load_config_file(ref_file) ref_pars = { par: value for par, value in ref.items() if par not in ["class", "name"] } logger.debug( - f"{reftype.upper()} parameters retrieved from CRDS: {ref_pars}" + "%s parameters retrieved from CRDS: %s", reftype.upper(), ref_pars ) return ref - else: - logger.debug(f"No {reftype.upper()} reference files found.") - return config_parser.ConfigObj() + + logger.debug("No %s reference files found.", reftype.upper()) + return config_parser.ConfigObj() @classmethod def reference_uri_to_cache_path(cls, reference_uri, observatory): @@ -923,7 +937,7 @@ def save_model( idx=None, output_file=None, force=False, - format=None, + format=None, # noqa: A002 **components, ): """ @@ -968,7 +982,7 @@ def save_model( # Check if saving is even specified. if not force and not self.save_results and not output_file: - return + return None if isinstance(model, Sequence): save_model_func = partial( @@ -998,7 +1012,7 @@ def save_model( **components, ) ) - self.log.info(f"Saved model in {output_path}") + self.log.info("Saved model in %s", output_path) return output_path @@ -1122,9 +1136,7 @@ def _make_output_path( output_dir = step.search_attr("output_dir", default="") output_dir = expandvars(expanduser(output_dir)) - full_output_path = join(output_dir, basename) - - return full_output_path + return join(output_dir, basename) def closeout(self, to_close=None, to_del=None): """Close out step processing @@ -1152,12 +1164,12 @@ def closeout(self, to_close=None, to_del=None): try: if hasattr(item, "close"): item.close() - except Exception as exception: - self.log.debug(f'Could not close "{item}"Reason:\n{exception}') + except Exception as exception: # noqa: PERF203 + self.log.debug('Could not close "%s"Reason:\n%s', item, exception) for item in to_del: try: del item - except NameError as error: + except NameError as error: # noqa: PERF203 self.log.debug("An error has occurred: %s", error) gc.collect() @@ -1220,7 +1232,7 @@ def make_input_path(self, file_path): return full_path - def _set_input_dir(self, input, exclusive=True): + def _set_input_dir(self, input_, exclusive=True): """Set the input directory If sufficient information is at hand, set a value @@ -1228,7 +1240,7 @@ def _set_input_dir(self, input, exclusive=True): Parameters ---------- - input : str + input_ : str Input to determine path from. exclusive : bool @@ -1237,12 +1249,9 @@ def _set_input_dir(self, input, exclusive=True): """ if not exclusive or self.search_attr("_input_dir") is None: - try: - if isfile(input): - self.input_dir = split(input)[0] - except Exception: - # Not a file-checkable object. Ignore. - pass + with suppress(Exception): + if isfile(input_): + self.input_dir = split(input_)[0] def get_pars(self, full_spec=True): """Retrieve the configuration parameters of a step @@ -1330,11 +1339,11 @@ def update_pars(self, parameters): getattr(self, step_name).update_pars(step_parameters) else: self.log.debug( - f"Parameter {parameter} is not valid for step {self}. Ignoring." + "Parameter %s is not valid for step %s. Ignoring.", parameter, self ) @classmethod - def build_config(cls, input, **kwargs): + def build_config(cls, input, **kwargs): # noqa: A002 """Build the ConfigObj to initialize a Step A Step config is built in the following order: @@ -1461,8 +1470,10 @@ def get_disable_crds_steppars(default=None): if default: if isinstance(default, bool): return default - elif isinstance(default, str): + + if isinstance(default, str): return default in truths + raise ValueError(f"default must be string or boolean: {default}") flag = os.environ.get("STPIPE_DISABLE_CRDS_STEPPARS", "") diff --git a/src/stpipe/subproc.py b/src/stpipe/subproc.py index fb83e861..e21e2eab 100644 --- a/src/stpipe/subproc.py +++ b/src/stpipe/subproc.py @@ -29,7 +29,7 @@ class SystemCall(Step): """ # noqa: E501 def process(self, *args): - from .. import datamodels + from .. import datamodels # noqa: TID252 newargs = [] for i, arg in enumerate(args): @@ -48,7 +48,7 @@ def process(self, *args): env[var] = val or None # Start the process and wait for it to finish. - self.log.info(f"Spawning {cmd_str!r}") + self.log.info("Spawning %r", cmd_str) try: p = subprocess.Popen( args=[cmd_str], @@ -60,19 +60,18 @@ def process(self, *args): ) err = p.wait() except Exception as e: - msg = f"Failed with an exception: \n{e}" - self.log.info(msg) + self.log.info("Failed with an exception: \n%s", e) if self.failure_as_exception: raise else: - self.log.info(f"Done with errorcode {err}") + self.log.info("Done with errorcode %s", err) # Log STDOUT/ERR if we are asked to do so. if self.log_stdout: - self.log.info(f"STDOUT: {p.stdout.read()}") + self.log.info("STDOUT: %s", p.stdout.read()) if self.log_stderr: - self.log.info(f"STDERR: {p.stderr.read()}") + self.log.info("STDERR: %s", p.stderr.read()) if self.exitcode_as_exception and err != 0: raise OSError(f"{cmd_str!r} returned error code {err}") diff --git a/src/stpipe/utilities.py b/src/stpipe/utilities.py index d267edc4..cf615a1a 100644 --- a/src/stpipe/utilities.py +++ b/src/stpipe/utilities.py @@ -45,7 +45,7 @@ def import_class(full_name, subclassof=object, config_file=None): # package.subPackage.subsubpackage.className # in the input parameter `full_name`. This means that # 1. We HAVE to be able to say - # from package.subPackage.subsubpackage import className + # `from package.subPackage.subsubpackage import className` # 2. If `subclassof` is defined, the newly imported Python class MUST be a # subclass of `subclassof`, which HAS to be a Python class. @@ -73,7 +73,8 @@ def import_class(full_name, subclassof=object, config_file=None): raise TypeError( f"Object {class_name} from package {package_name} is not a class" ) - elif not issubclass(step_class, subclassof): + + if not issubclass(step_class, subclassof): raise TypeError( f"Class {class_name} from package {package_name} is not a subclass of" f" {subclassof.__name__}" @@ -100,8 +101,8 @@ def get_spec_file_path(step_class): # Since `step_class` could be defined in a file called whatever, # we need the source file basedir and the class name. - dir = os.path.dirname(step_source_file) - return os.path.join(dir, step_class.__name__ + ".spec") + dir_ = os.path.dirname(step_source_file) + return os.path.join(dir_, step_class.__name__ + ".spec") def find_spec_file(step_class): @@ -133,8 +134,8 @@ def get_fully_qualified_class_name(cls_or_obj): module = cls.__module__ if module is None or module == str.__class__.__module__: return cls.__name__ # Avoid reporting __builtin__ - else: - return module + "." + cls.__name__ + + return module + "." + cls.__name__ class _NotSet: @@ -144,7 +145,5 @@ class _NotSet: below """ - pass - _not_set = _NotSet() diff --git a/tests/cli/test_list.py b/tests/cli/test_list.py index 8ba721f0..6bb3807d 100644 --- a/tests/cli/test_list.py +++ b/tests/cli/test_list.py @@ -6,7 +6,7 @@ @pytest.fixture(autouse=True) -def monkey_patch_get_steps(monkeypatch): +def _monkey_patch_get_steps(monkeypatch): def _get_steps(): return [ StepInfo( diff --git a/tests/cli/test_main.py b/tests/cli/test_main.py index b941b121..12c09ef4 100644 --- a/tests/cli/test_main.py +++ b/tests/cli/test_main.py @@ -9,7 +9,7 @@ @pytest.fixture(autouse=True) -def monkey_patch_get_steps(monkeypatch): +def _monkey_patch_get_steps(monkeypatch): def _get_steps(): return [ StepInfo( diff --git a/tests/test_format_template.py b/tests/test_format_template.py index 47f771b1..106ffaa1 100644 --- a/tests/test_format_template.py +++ b/tests/test_format_template.py @@ -7,7 +7,7 @@ @pytest.mark.parametrize( - "key_formats, template, expected, fields, errors", + ("key_formats", "template", "expected", "fields", "errors"), [ # No replacement at all. ( diff --git a/tests/test_logger.py b/tests/test_logger.py index 03817dc8..432224ff 100644 --- a/tests/test_logger.py +++ b/tests/test_logger.py @@ -7,7 +7,7 @@ @pytest.fixture(autouse=True) -def clean_up_logging(): +def _clean_up_logging(): yield logging.shutdown() stpipe_log.load_configuration(io.BytesIO(stpipe_log.DEFAULT_CONFIGURATION)) diff --git a/tests/test_step.py b/tests/test_step.py index e61a0d59..22c80ef7 100644 --- a/tests/test_step.py +++ b/tests/test_step.py @@ -1,5 +1,7 @@ """Test step.Step""" import logging +import re +from typing import ClassVar import asdf import pytest @@ -36,7 +38,7 @@ class SimplePipe(Pipeline): output_ext = string(default='simplestep') """ - step_defs = {"step1": SimpleStep} + step_defs: ClassVar = {"step1": SimpleStep} class LoggingPipeline(Pipeline): @@ -52,8 +54,7 @@ class LoggingPipeline(Pipeline): def process(self): self.log.warning("This step has called out a warning.") - self.log.warning(f"{self.log} {self.log.handlers}") - return + self.log.warning("%s %s", self.log, self.log.handlers) def _datamodels_open(self, **kwargs): pass @@ -138,12 +139,12 @@ def config_file_list_arg_step(tmpdir): return config_file -@pytest.fixture -def mock_step_crds(monkeypatch): +@pytest.fixture() +def _mock_step_crds(monkeypatch): """Mock various crds calls from Step""" def mock_get_config_from_reference_pipe(dataset, disable=None): - config = cp.config_from_dict( + return cp.config_from_dict( { "str1": "from crds", "str2": "from crds", @@ -157,17 +158,14 @@ def mock_get_config_from_reference_pipe(dataset, disable=None): }, } ) - return config def mock_get_config_from_reference_step(dataset, disable=None): - config = cp.config_from_dict( + return cp.config_from_dict( {"str1": "from crds", "str2": "from crds", "str3": "from crds"} ) - return config def mock_get_config_from_reference_list_arg_step(dataset, disable=None): - config = cp.config_from_dict({"rotation": "15", "pixel_scale": "0.85"}) - return config + return cp.config_from_dict({"rotation": "15", "pixel_scale": "0.85"}) monkeypatch.setattr( SimplePipe, "get_config_from_reference", mock_get_config_from_reference_pipe @@ -185,7 +183,8 @@ def mock_get_config_from_reference_list_arg_step(dataset, disable=None): # ##### # Tests # ##### -def test_build_config_pipe_config_file(mock_step_crds, config_file_pipe): +@pytest.mark.usefixtures("_mock_step_crds") +def test_build_config_pipe_config_file(config_file_pipe): """Test that local config overrides defaults and CRDS-supplied file""" config, returned_config_file = SimplePipe.build_config( "science.fits", config_file=config_file_pipe @@ -199,7 +198,8 @@ def test_build_config_pipe_config_file(mock_step_crds, config_file_pipe): assert config["steps"]["step1"]["str3"] == "from crds" -def test_build_config_pipe_crds(mock_step_crds): +@pytest.mark.usefixtures("_mock_step_crds") +def test_build_config_pipe_crds(): """Test that CRDS param reffile overrides a default CRDS configuration""" config, config_file = SimplePipe.build_config("science.fits") assert not config_file @@ -218,7 +218,8 @@ def test_build_config_pipe_default(): assert len(config) == 0 -def test_build_config_pipe_kwarg(mock_step_crds, config_file_pipe): +@pytest.mark.usefixtures("_mock_step_crds") +def test_build_config_pipe_kwarg(config_file_pipe): """Test that kwargs override CRDS and local param reffiles""" config, returned_config_file = SimplePipe.build_config( "science.fits", @@ -235,7 +236,8 @@ def test_build_config_pipe_kwarg(mock_step_crds, config_file_pipe): assert config["steps"]["step1"]["str3"] == "from crds" -def test_build_config_step_config_file(mock_step_crds, config_file_step): +@pytest.mark.usefixtures("_mock_step_crds") +def test_build_config_step_config_file(config_file_step): """Test that local config overrides defaults and CRDS-supplied file""" config, returned_config_file = SimpleStep.build_config( "science.fits", config_file=config_file_step @@ -246,7 +248,8 @@ def test_build_config_step_config_file(mock_step_crds, config_file_step): assert config["str3"] == "from crds" -def test_build_config_step_crds(mock_step_crds): +@pytest.mark.usefixtures("_mock_step_crds") +def test_build_config_step_crds(): """Test override of a CRDS configuration""" config, config_file = SimpleStep.build_config("science.fits") assert config_file is None @@ -263,7 +266,8 @@ def test_build_config_step_default(): assert len(config) == 0 -def test_build_config_step_kwarg(mock_step_crds, config_file_step): +@pytest.mark.usefixtures("_mock_step_crds") +def test_build_config_step_kwarg(config_file_step): """Test that kwargs override everything""" config, returned_config_file = SimpleStep.build_config( "science.fits", config_file=config_file_step, str1="from kwarg" @@ -274,7 +278,8 @@ def test_build_config_step_kwarg(mock_step_crds, config_file_step): assert config["str3"] == "from crds" -def test_step_list_args(mock_step_crds, config_file_list_arg_step): +@pytest.mark.usefixtures("_mock_step_crds") +def test_step_list_args(config_file_list_arg_step): """Test that list arguments, provided as comma-separated values are parsed correctly. """ @@ -301,7 +306,11 @@ def test_step_list_args(mock_step_crds, config_file_list_arg_step): assert c.output_shape == [1500, 1300] assert c.crpix == [123, 456] - with pytest.raises(ValueError) as e: + msg = re.escape( + "Config parameter 'output_shape': the value \"['1500', '1300', '90']\" " + "is too long." + ) + with pytest.raises(ValueError, match=msg): cmdline.just_the_step_from_cmdline( [ "filename.fits", @@ -314,13 +323,11 @@ def test_step_list_args(mock_step_crds, config_file_list_arg_step): ], ListArgStep, ) - assert ( - e.value.args[0] - == "Config parameter 'output_shape': the value \"['1500', '1300', '90']\" is" - " too long." - ) - with pytest.raises(ValueError) as e: + msg = re.escape( + "Config parameter 'output_shape': the value \"['1500']\" is too short." + ) + with pytest.raises(ValueError, match=msg): cmdline.just_the_step_from_cmdline( [ "filename.fits", @@ -333,12 +340,11 @@ def test_step_list_args(mock_step_crds, config_file_list_arg_step): ], ListArgStep, ) - assert ( - e.value.args[0] - == "Config parameter 'output_shape': the value \"['1500']\" is too short." - ) - with pytest.raises(ValueError) as e: + msg = re.escape( + "Config parameter 'output_shape': the value \"1500\" is of the wrong type." + ) + with pytest.raises(ValueError, match=msg): cmdline.just_the_step_from_cmdline( [ "filename.fits", @@ -351,12 +357,11 @@ def test_step_list_args(mock_step_crds, config_file_list_arg_step): ], ListArgStep, ) - assert ( - e.value.args[0] - == "Config parameter 'output_shape': the value \"1500\" is of the wrong type." - ) - with pytest.raises(ValueError) as e: + msg = re.escape( + "Config parameter 'output_shape': the value \"1500.5\" is of the wrong type." + ) + with pytest.raises(ValueError, match=msg): cmdline.just_the_step_from_cmdline( [ "filename.fits", @@ -369,10 +374,6 @@ def test_step_list_args(mock_step_crds, config_file_list_arg_step): ], ListArgStep, ) - assert ( - e.value.args[0] - == "Config parameter 'output_shape': the value \"1500.5\" is of the wrong type." - ) def test_logcfg_routing(tmpdir): @@ -394,7 +395,7 @@ def test_logcfg_routing(tmpdir): handler.close() with open(tmpdir / "myrun.log") as f: - fulltext = "\n".join([line for line in f]) + fulltext = "\n".join(list(f)) assert "called out a warning" in fulltext