From 8dc535d018cda278d580d5bd5585f97c44d06a5d Mon Sep 17 00:00:00 2001 From: Jan Snasel Date: Mon, 12 Feb 2024 10:37:22 +0000 Subject: [PATCH 1/3] feat: Use version constraints in individual test cases --- pylint_nautobot/__init__.py | 7 +- pylint_nautobot/incorrect_base_class.py | 36 ++----- pylint_nautobot/use_fields_all.py | 13 ++- pylint_nautobot/utils.py | 129 +++++++++++++++++++----- tests/test_deprecated_status_model.py | 17 ++-- tests/test_incorrect_base_class.py | 27 +++-- tests/test_string_field_blank_null.py | 17 ++-- tests/test_use_fields_all.py | 17 ++-- tests/utils.py | 52 ++++++++-- 9 files changed, 207 insertions(+), 108 deletions(-) diff --git a/pylint_nautobot/__init__.py b/pylint_nautobot/__init__.py index 34c52af..6899162 100644 --- a/pylint_nautobot/__init__.py +++ b/pylint_nautobot/__init__.py @@ -1,7 +1,7 @@ """Initialization file for library.""" + from importlib import metadata -from packaging.specifiers import SpecifierSet from pylint.lint import PyLinter from .code_location_changes import NautobotCodeLocationChangesChecker @@ -11,7 +11,7 @@ from .replaced_models import NautobotReplacedModelsImportChecker from .string_field_blank_null import NautobotStringFieldBlankNull from .use_fields_all import NautobotUseFieldsAllChecker -from .utils import MINIMUM_NAUTOBOT_VERSION +from .utils import is_version_compatible __version__ = metadata.version(__name__) @@ -29,6 +29,5 @@ def register(linter: PyLinter): """Pylint plugin entrypoint - register all the checks to the linter.""" for checker in CHECKERS: - checker_versions = SpecifierSet(checker.version_specifier) - if not checker_versions or MINIMUM_NAUTOBOT_VERSION in checker_versions: + if is_version_compatible(checker.version_specifier): linter.register_checker(checker(linter)) diff --git a/pylint_nautobot/incorrect_base_class.py b/pylint_nautobot/incorrect_base_class.py index 24beafe..5051d3d 100644 --- a/pylint_nautobot/incorrect_base_class.py +++ b/pylint_nautobot/incorrect_base_class.py @@ -1,29 +1,9 @@ """Check for imports whose paths have changed in 2.0.""" -from astroid import Assign -from astroid import ClassDef -from astroid import Const -from pylint.checkers import BaseChecker - -from pylint_nautobot.utils import is_nautobot_v2_installed +from pylint.checkers import BaseChecker -def is_abstract(node): - """Given a node, returns whether it is an abstract base model.""" - for child_node in node.get_children(): - if not (isinstance(child_node, ClassDef) and child_node.name == "Meta"): - continue - for meta_child in child_node.get_children(): - if ( - not isinstance(meta_child, Assign) - or not meta_child.targets[0].name == "abstract" - or not isinstance(meta_child.value, Const) - ): - continue - # At this point we know we are dealing with an assignment to a constant for the 'abstract' field on the - # 'Meta' class. Therefore, we can assume the value of that to be whether the node is an abstract base model - # or not. - return meta_child.value.value - return False +from .utils import is_abstract_class +from .utils import is_version_compatible class NautobotIncorrectBaseClassChecker(BaseChecker): @@ -41,9 +21,11 @@ class NautobotIncorrectBaseClassChecker(BaseChecker): ("django.db.models.base.Model", "nautobot.core.models.BaseModel"), ( "django.forms.forms.Form", - "nautobot.core.forms.forms.BootstrapMixin" - if is_nautobot_v2_installed() - else "nautobot.utilities.forms.forms.BootstrapMixin", + ( + "nautobot.core.forms.forms.BootstrapMixin" + if is_version_compatible(">=2") + else "nautobot.utilities.forms.forms.BootstrapMixin" + ), ), ] @@ -58,7 +40,7 @@ class NautobotIncorrectBaseClassChecker(BaseChecker): def visit_classdef(self, node): """Visit class definitions.""" - if is_abstract(node): + if is_abstract_class(node): return # Skip mixin classes diff --git a/pylint_nautobot/use_fields_all.py b/pylint_nautobot/use_fields_all.py index 52676de..1ee40cd 100644 --- a/pylint_nautobot/use_fields_all.py +++ b/pylint_nautobot/use_fields_all.py @@ -1,4 +1,5 @@ """Check for CharField's or TextField's on models where null=True and blank=True.""" + from astroid import Assign from astroid import AssignName from astroid import ClassDef @@ -15,8 +16,6 @@ "nautobot.utilities.tables.BaseTable.Meta": ">1,<2", } -_CHECK_CLASSES = [key for key, specifier_set in _META_CLASSES.items() if is_version_compatible(specifier_set)] - class NautobotUseFieldsAllChecker(BaseChecker): """Visit Meta subclasses and check for use of `fields = "__all__"`, instead of `fields = ["field1", ...]`.""" @@ -34,9 +33,17 @@ class NautobotUseFieldsAllChecker(BaseChecker): ), } + def __init__(self, *args, **kwargs): + """Initialize the checker.""" + super().__init__(*args, **kwargs) + + self.meta_classes = [ + key for key, specifier_set in _META_CLASSES.items() if is_version_compatible(specifier_set) + ] + def visit_classdef(self, node: ClassDef): """Visit class definitions.""" - if not any(ancestor.qname() in _CHECK_CLASSES for ancestor in node.ancestors()): + if not any(ancestor.qname() in self.meta_classes for ancestor in node.ancestors()): return for child_node in node.get_children(): diff --git a/pylint_nautobot/utils.py b/pylint_nautobot/utils.py index 945f990..2a4f911 100644 --- a/pylint_nautobot/utils.py +++ b/pylint_nautobot/utils.py @@ -1,40 +1,23 @@ """Utilities for managing data.""" + from importlib import metadata from pathlib import Path +from typing import List from typing import Optional from typing import Union import toml +from astroid import Assign +from astroid import Attribute +from astroid import ClassDef +from astroid import Const +from astroid import Name from importlib_resources import files from packaging.specifiers import SpecifierSet from packaging.version import Version from yaml import safe_load -def is_nautobot_v2_installed() -> bool: - """Return True if Nautobot v2.x is installed.""" - return MINIMUM_NAUTOBOT_VERSION.major == 2 - - -def is_version_compatible(specifier_set: Union[str, SpecifierSet]) -> bool: - """Return True if the Nautobot version is compatible with the given version specifier_set.""" - if isinstance(specifier_set, str): - specifier_set = SpecifierSet(specifier_set) - return specifier_set.contains(MINIMUM_NAUTOBOT_VERSION) - - -def load_v2_code_location_changes(): - """Black magic data transform, needs schema badly.""" - with open(files("pylint_nautobot") / "data" / "v2" / "v2-code-location-changes.yaml", encoding="utf-8") as rules: - changes = safe_load(rules) - changes_map = {} - for change in changes: - if change["Old Module"] not in changes_map: - changes_map[change["Old Module"]] = {} - changes_map[change["Old Module"]][change["Class/Function(s)"]] = change["New Module"] - return changes_map - - def _read_poetry_lock() -> dict: for directory in (Path.cwd(), *Path.cwd().parents): path = directory / "poetry.lock" @@ -56,5 +39,101 @@ def _read_locked_nautobot_version() -> Optional[str]: return None -MAP_CODE_LOCATION_CHANGES = load_v2_code_location_changes() MINIMUM_NAUTOBOT_VERSION = Version(_read_locked_nautobot_version() or metadata.version("nautobot")) + + +def is_abstract_class(node: ClassDef) -> bool: + """Given a node, returns whether it is an abstract base model.""" + for child_node in node.get_children(): + if not (isinstance(child_node, ClassDef) and child_node.name == "Meta"): + continue + + for meta_child in child_node.get_children(): + if ( + not isinstance(meta_child, Assign) + or not meta_child.targets[0].name == "abstract" # type: ignore + or not isinstance(meta_child.value, Const) + ): + continue + # At this point we know we are dealing with an assignment to a constant for the 'abstract' field on the + # 'Meta' class. Therefore, we can assume the value of that to be whether the node is an abstract base model + # or not. + return meta_child.value.value + + return False + + +def get_model_name(ancestor: str, node: ClassDef) -> str: + """Get the model name from the class definition.""" + if ancestor == "from nautobot.apps.views.NautobotUIViewSet": + raise NotImplementedError("This ancestor is not yet supported.") + + meta = next((n for n in node.body if isinstance(n, ClassDef) and n.name == "Meta"), None) + if not meta: + raise NotImplementedError("This class does not have a Meta class.") + + model_attr = next( + ( + attr + for attr in meta.body + if isinstance(attr, Assign) + and any( + isinstance(target, (Name, Attribute)) + and getattr(target, "attrname", None) == "model" + or getattr(target, "name", None) == "model" + for target in attr.targets + ) + ), + None, + ) + if not model_attr: + raise NotImplementedError("The Meta class does not define a model attribute.") + + if isinstance(model_attr.value, Name): + return model_attr.value.name + if not isinstance(model_attr.value, Attribute): + raise NotImplementedError("This utility supports only direct assignment or attribute based model names.") + + model_attr_chain = [] + while isinstance(model_attr.value, Attribute): + model_attr_chain.insert(0, model_attr.value.attrname) + model_attr.value = model_attr.value.expr + + if isinstance(model_attr.value, Name): + model_attr_chain.insert(0, model_attr.value.name) + + return model_attr_chain[-1] + + +def find_ancestor(node: ClassDef, ancestors: List[str]) -> str: + """Find the class ancestor from the list of ancestors.""" + ancestor_class_types = [ancestor.qname() for ancestor in node.ancestors()] + for checked_ancestor in ancestors: + if checked_ancestor in ancestor_class_types: + return checked_ancestor + + return "" + + +def is_version_compatible(specifier_set: Union[str, SpecifierSet]) -> bool: + """Return True if the Nautobot version is compatible with the given version specifier_set.""" + if not specifier_set: + return True + if isinstance(specifier_set, str): + specifier_set = SpecifierSet(specifier_set) + return specifier_set.contains(MINIMUM_NAUTOBOT_VERSION) + + +def load_v2_code_location_changes(): + """Black magic data transform, needs schema badly.""" + with open(files("pylint_nautobot") / "data" / "v2" / "v2-code-location-changes.yaml", encoding="utf-8") as rules: # type: ignore + changes = safe_load(rules) + changes_map = {} + for change in changes: + if change["Old Module"] not in changes_map: + changes_map[change["Old Module"]] = {} + changes_map[change["Old Module"]][change["Class/Function(s)"]] = change["New Module"] + return changes_map + + +MAP_CODE_LOCATION_CHANGES = load_v2_code_location_changes() diff --git a/tests/test_deprecated_status_model.py b/tests/test_deprecated_status_model.py index 01e86f5..39a5b73 100644 --- a/tests/test_deprecated_status_model.py +++ b/tests/test_deprecated_status_model.py @@ -1,15 +1,14 @@ """Tests for deprecated status model.""" -from pathlib import Path from pylint.testutils import CheckerTestCase -from pytest import mark from pylint_nautobot.deprecated_status_model import NautobotDeprecatedStatusModelChecker from .utils import assert_error_file from .utils import assert_good_file +from .utils import parametrize_error_files +from .utils import parametrize_good_files -_INPUTS_PATH = Path(__file__).parent / "inputs/deprecated-status-model/" _EXPECTED_ERRORS = { "status_model": { "msg_id": "nb-status-field-instead-of-status-model", @@ -25,11 +24,11 @@ class TestDeprecatedStatusModelChecker(CheckerTestCase): CHECKER_CLASS = NautobotDeprecatedStatusModelChecker - @mark.parametrize("path", _INPUTS_PATH.glob("error_*.py")) - def test_deprecated_status_model(self, path): - assert_error_file(self, path, _EXPECTED_ERRORS) + @parametrize_error_files(__file__, _EXPECTED_ERRORS) + def test_error(self, filename, expected_error): + assert_error_file(self, filename, expected_error) # TBD: Missing test for good_status_model.py - @mark.parametrize("path", _INPUTS_PATH.glob("good_*.py")) - def test_no_issues(self, path): - assert_good_file(self, path) + @parametrize_good_files(__file__) + def test_good(self, filename): + assert_good_file(self, filename) diff --git a/tests/test_incorrect_base_class.py b/tests/test_incorrect_base_class.py index c536a63..a6c7e64 100644 --- a/tests/test_incorrect_base_class.py +++ b/tests/test_incorrect_base_class.py @@ -1,21 +1,26 @@ """Tests for incorrect base class checker.""" -from pathlib import Path from pylint.testutils import CheckerTestCase -from pytest import mark from pylint_nautobot.incorrect_base_class import NautobotIncorrectBaseClassChecker from .utils import assert_error_file from .utils import assert_good_file +from .utils import parametrize_error_files +from .utils import parametrize_good_files -_INPUTS_PATH = Path(__file__).parent / "inputs/incorrect-base-class" -_EXPECTED_ERROR_ARGS = { + +def _find_error_node(module_node): + return module_node.body[1] + + +_EXPECTED_ERRORS = { "model": { + "versions": ">=2", "msg_id": "nb-incorrect-base-class", "line": 4, "col_offset": 0, - "node": lambda module_node: module_node.body[1], + "node": _find_error_node, "args": ("django.db.models.base.Model", "nautobot.core.models.BaseModel"), }, } @@ -26,10 +31,10 @@ class TestIncorrectBaseClassChecker(CheckerTestCase): CHECKER_CLASS = NautobotIncorrectBaseClassChecker - @mark.parametrize("path", _INPUTS_PATH.glob("error_*.py")) - def test_incorrect_base_class(self, path): - assert_error_file(self, path, _EXPECTED_ERROR_ARGS) + @parametrize_error_files(__file__, _EXPECTED_ERRORS) + def test_error(self, filename, expected_error): + assert_error_file(self, filename, expected_error) - @mark.parametrize("path", _INPUTS_PATH.glob("good_*.py")) - def test_no_issues(self, path): - assert_good_file(self, path) + @parametrize_good_files(__file__) + def test_good(self, filename): + assert_good_file(self, filename) diff --git a/tests/test_string_field_blank_null.py b/tests/test_string_field_blank_null.py index d016ed0..ffdb99f 100644 --- a/tests/test_string_field_blank_null.py +++ b/tests/test_string_field_blank_null.py @@ -1,15 +1,14 @@ """Tests for blank and null StringFields.""" -from pathlib import Path from pylint.testutils import CheckerTestCase -from pytest import mark from pylint_nautobot.string_field_blank_null import NautobotStringFieldBlankNull from .utils import assert_error_file from .utils import assert_good_file +from .utils import parametrize_error_files +from .utils import parametrize_good_files -_INPUTS_PATH = Path(__file__).parent / "inputs/string-field-blank-null/" _EXPECTED_ERRORS = { "field": { "msg_id": "nb-string-field-blank-null", @@ -25,10 +24,10 @@ class TestStringFieldBlankNullChecker(CheckerTestCase): CHECKER_CLASS = NautobotStringFieldBlankNull - @mark.parametrize("path", _INPUTS_PATH.glob("error_*.py")) - def test_string_field_blank_null(self, path): - assert_error_file(self, path, _EXPECTED_ERRORS) + @parametrize_error_files(__file__, _EXPECTED_ERRORS) + def test_error(self, filename, expected_error): + assert_error_file(self, filename, expected_error) - @mark.parametrize("path", _INPUTS_PATH.glob("good_*.py")) - def test_no_issues(self, path): - assert_good_file(self, path) + @parametrize_good_files(__file__) + def test_good(self, filename): + assert_good_file(self, filename) diff --git a/tests/test_use_fields_all.py b/tests/test_use_fields_all.py index 88c302e..073a442 100644 --- a/tests/test_use_fields_all.py +++ b/tests/test_use_fields_all.py @@ -1,13 +1,13 @@ """Tests for use fields all""" -from pathlib import Path from pylint.testutils import CheckerTestCase -from pytest import mark from pylint_nautobot.use_fields_all import NautobotUseFieldsAllChecker from .utils import assert_error_file from .utils import assert_good_file +from .utils import parametrize_error_files +from .utils import parametrize_good_files def _find_fields_node(module_node): @@ -17,7 +17,6 @@ def _find_fields_node(module_node): return list(meta.get_children())[1].value -_INPUTS_PATH = Path(__file__).parent / "inputs/use-fields-all/" _EXPECTED_ERRORS = { "table": { "msg_id": "nb-use-fields-all", @@ -33,10 +32,10 @@ class TestUseFieldsAllChecker(CheckerTestCase): CHECKER_CLASS = NautobotUseFieldsAllChecker - @mark.parametrize("path", _INPUTS_PATH.glob("error_*.py")) - def test_use_fields_all(self, path): - assert_error_file(self, path, _EXPECTED_ERRORS) + @parametrize_error_files(__file__, _EXPECTED_ERRORS) + def test_error(self, filename, expected_error): + assert_error_file(self, filename, expected_error) - @mark.parametrize("path", _INPUTS_PATH.glob("good_*.py")) - def test_no_issues(self, path): - assert_good_file(self, path) + @parametrize_good_files(__file__) + def test_good(self, filename): + assert_good_file(self, filename) diff --git a/tests/utils.py b/tests/utils.py index d97af1d..bd89f9b 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,6 +1,43 @@ """Test utilities.""" + +from pathlib import Path + import astroid from pylint.testutils import MessageTest +from pytest import mark + +from pylint_nautobot.utils import is_version_compatible + +_INPUTS_PATH = Path(__file__).parent / "inputs" + + +def get_checker_name(module): + return Path(module).stem[5:].replace("_", "-") + + +def parametrize_good_files(module): + checker_name = get_checker_name(module) + return mark.parametrize( + "filename", + [f"{checker_name}/{item.name}" for item in (_INPUTS_PATH / checker_name).glob("good_*.py")], + ) + + +def parametrize_error_files(module, expected_errors): + checker_name = get_checker_name(module) + inputs_path = _INPUTS_PATH / checker_name + names = set(item.stem[6:] for item in (inputs_path).glob("error_*.py")) | set(expected_errors) + + def get_params(): + for name in names: + if name not in expected_errors: + raise ValueError(f"Missing expected error for {name} in {module}") + expected_error = {**expected_errors[name]} + versions = expected_error.pop("versions", "") + if is_version_compatible(versions): + yield f"{checker_name}/error_{name}.py", expected_error + + return mark.parametrize(("filename", "expected_error"), get_params()) def assert_no_message(test_case, test_code): @@ -10,12 +47,12 @@ def assert_no_message(test_case, test_code): test_case.walk(module_node) -def assert_good_file(test_case, path): +def assert_good_file(test_case, filename): """Assert that no message is emitted for the given file.""" - assert_no_message(test_case, path.read_text(encoding="utf-8")) + assert_no_message(test_case, (_INPUTS_PATH / filename).read_text(encoding="utf-8")) -def assert_error_file(test_case, path, expected_errors): +def assert_error_file(test_case, filename, expected_error): """Assert that the given messages are emitted for the given file. Args: @@ -26,15 +63,8 @@ def assert_error_file(test_case, path, expected_errors): e.g. `error_status_model.py` becomes `status_model` key in the dictionary. """ - name = path.name[6:-3] - test_code = path.read_text(encoding="utf-8") + test_code = (_INPUTS_PATH / filename).read_text(encoding="utf-8") module_node = astroid.parse(test_code) - try: - expected_error = expected_errors[name] - except KeyError: - raise KeyError( - f"Missing expected error args for {name}. Perhaps you need to add it to `expected_errors`?" - ) from None node = expected_error.pop("node") with test_case.assertAddsMessages( MessageTest( From 6467cb38d3224c9d8e4aae504741142bdd31dbaa Mon Sep 17 00:00:00 2001 From: Jan Snasel Date: Mon, 12 Feb 2024 11:07:02 +0000 Subject: [PATCH 2/3] chore: Ignore `/var` directory --- .flake8 | 1 + pyproject.toml | 1 + 2 files changed, 2 insertions(+) diff --git a/.flake8 b/.flake8 index 5912a6b..d4e2c4c 100644 --- a/.flake8 +++ b/.flake8 @@ -6,3 +6,4 @@ exclude = __pycache__, .venv, tests/inputs + var diff --git a/pyproject.toml b/pyproject.toml index 682e389..68042ec 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -66,6 +66,7 @@ exclude = ''' | buck-out | build | dist + | var )/ | settings.py # This is where you define files that should not be stylized by black # the root of the project From 8c3d4d783b32e6b87034b13f11bdfd5174d2c81b Mon Sep 17 00:00:00 2001 From: Jan Snasel Date: Mon, 12 Feb 2024 11:07:16 +0000 Subject: [PATCH 3/3] feat: Added `--pull` option for `invoke build` --- tasks.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tasks.py b/tasks.py index 9ff8b86..0bea887 100644 --- a/tasks.py +++ b/tasks.py @@ -146,9 +146,10 @@ def run_command(context, command, **kwargs): help={ "force_rm": "Always remove intermediate containers", "cache": "Whether to use Docker's cache when building the image (defaults to enabled)", + "pull": "Always attempt to pull a newer version of the base image", } ) -def build(context, force_rm=False, cache=True): +def build(context, force_rm=False, cache=True, pull=False): """Build Nautobot docker image.""" command = "build" @@ -156,6 +157,8 @@ def build(context, force_rm=False, cache=True): command += " --no-cache" if force_rm: command += " --force-rm" + if pull: + command += " --pull" print(f"Building Nautobot with Python {context.pylint_nautobot.python_ver}...") docker_compose(context, command)