From 992097b8f40e6a66ba6511c8bb475b3b60552299 Mon Sep 17 00:00:00 2001 From: Jan Snasel Date: Fri, 9 Feb 2024 12:57:00 +0000 Subject: [PATCH 1/7] feat: Limit tests with version constraints --- pylint_nautobot/utils.py | 4 ++- tests/test_deprecated_status_model.py | 13 +++++---- tests/test_incorrect_base_class.py | 15 +++++------ tests/test_string_field_blank_null.py | 13 +++++---- tests/test_use_fields_all.py | 13 +++++---- tests/utils.py | 38 +++++++++++++++++++++------ 6 files changed, 58 insertions(+), 38 deletions(-) diff --git a/pylint_nautobot/utils.py b/pylint_nautobot/utils.py index 945f990..f4eb89e 100644 --- a/pylint_nautobot/utils.py +++ b/pylint_nautobot/utils.py @@ -18,6 +18,8 @@ def is_nautobot_v2_installed() -> bool: 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) @@ -25,7 +27,7 @@ def is_version_compatible(specifier_set: Union[str, SpecifierSet]) -> bool: 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: + 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: diff --git a/tests/test_deprecated_status_model.py b/tests/test_deprecated_status_model.py index 01e86f5..21a3045 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_sub_class_name(self, path, expected_error): + assert_error_file(self, path, expected_error) # TBD: Missing test for good_status_model.py - @mark.parametrize("path", _INPUTS_PATH.glob("good_*.py")) + @parametrize_good_files(__file__) def test_no_issues(self, path): assert_good_file(self, path) diff --git a/tests/test_incorrect_base_class.py b/tests/test_incorrect_base_class.py index c536a63..8dab466 100644 --- a/tests/test_incorrect_base_class.py +++ b/tests/test_incorrect_base_class.py @@ -1,16 +1,15 @@ """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 = { +_EXPECTED_ERRORS = { "model": { "msg_id": "nb-incorrect-base-class", "line": 4, @@ -26,10 +25,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_sub_class_name(self, path, expected_error): + assert_error_file(self, path, expected_error) - @mark.parametrize("path", _INPUTS_PATH.glob("good_*.py")) + @parametrize_good_files(__file__) def test_no_issues(self, path): assert_good_file(self, path) diff --git a/tests/test_string_field_blank_null.py b/tests/test_string_field_blank_null.py index d016ed0..eb8f6a2 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_sub_class_name(self, path, expected_error): + assert_error_file(self, path, expected_error) - @mark.parametrize("path", _INPUTS_PATH.glob("good_*.py")) + @parametrize_good_files(__file__) def test_no_issues(self, path): assert_good_file(self, path) diff --git a/tests/test_use_fields_all.py b/tests/test_use_fields_all.py index 88c302e..8f360ee 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_sub_class_name(self, path, expected_error): + assert_error_file(self, path, expected_error) - @mark.parametrize("path", _INPUTS_PATH.glob("good_*.py")) + @parametrize_good_files(__file__) def test_no_issues(self, path): assert_good_file(self, path) diff --git a/tests/utils.py b/tests/utils.py index d97af1d..b1e3b56 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,6 +1,35 @@ """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 + + +def parametrize_good_files(module): + path = Path(module) + checker_name = path.stem[5:].replace("_", "-") + return mark.parametrize("path", (path.parent / "inputs" / checker_name).glob("good_*.py")) + + +def parametrize_error_files(module, expected_errors): + path = Path(module) + checker_name = path.stem[5:].replace("_", "-") + inputs_path = path.parent / "inputs" / 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: + expected_error = {**expected_errors[name]} + version = expected_error.pop("version", "") + if is_version_compatible(version): + path = inputs_path / f"error_{name}.py" + yield path, expected_error + + return mark.parametrize(("path", "expected_error"), get_params()) def assert_no_message(test_case, test_code): @@ -15,7 +44,7 @@ def assert_good_file(test_case, path): assert_no_message(test_case, path.read_text(encoding="utf-8")) -def assert_error_file(test_case, path, expected_errors): +def assert_error_file(test_case, path, expected_error): """Assert that the given messages are emitted for the given file. Args: @@ -26,15 +55,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") 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 1dc945e2a05f24fbd35e097ca2f4f33e16df33af Mon Sep 17 00:00:00 2001 From: Jan Snasel Date: Fri, 9 Feb 2024 13:06:45 +0000 Subject: [PATCH 2/7] chore: Don't check ignored `/var` --- .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 1be26765382b10a8d4a3e9e48c67187f053a9805 Mon Sep 17 00:00:00 2001 From: Jan Snasel Date: Fri, 9 Feb 2024 13:07:11 +0000 Subject: [PATCH 3/7] feat: Check sub class names --- pylint_nautobot/__init__.py | 2 + pylint_nautobot/sub_class_name.py | 44 ++++++++++++++ pylint_nautobot/utils.py | 58 +++++++++++++++++++ .../inputs/sub-class-name/error_filter_set.py | 15 +++++ .../inputs/sub-class-name/good_filter_set.py | 15 +++++ tests/test_sub_class_name.py | 39 +++++++++++++ 6 files changed, 173 insertions(+) create mode 100644 pylint_nautobot/sub_class_name.py create mode 100644 tests/inputs/sub-class-name/error_filter_set.py create mode 100644 tests/inputs/sub-class-name/good_filter_set.py create mode 100644 tests/test_sub_class_name.py diff --git a/pylint_nautobot/__init__.py b/pylint_nautobot/__init__.py index 34c52af..d3fec8e 100644 --- a/pylint_nautobot/__init__.py +++ b/pylint_nautobot/__init__.py @@ -10,6 +10,7 @@ from .model_label import NautobotModelLabelChecker from .replaced_models import NautobotReplacedModelsImportChecker from .string_field_blank_null import NautobotStringFieldBlankNull +from .sub_class_name import NautobotSubClassNameChecker from .use_fields_all import NautobotUseFieldsAllChecker from .utils import MINIMUM_NAUTOBOT_VERSION @@ -22,6 +23,7 @@ NautobotModelLabelChecker, NautobotReplacedModelsImportChecker, NautobotStringFieldBlankNull, + NautobotSubClassNameChecker, NautobotUseFieldsAllChecker, ] diff --git a/pylint_nautobot/sub_class_name.py b/pylint_nautobot/sub_class_name.py new file mode 100644 index 0000000..648759e --- /dev/null +++ b/pylint_nautobot/sub_class_name.py @@ -0,0 +1,44 @@ +"""Check for imports whose paths have changed in 2.0.""" + +from astroid import ClassDef +from pylint.checkers import BaseChecker + +from .utils import find_ancestor +from .utils import get_model_name +from .utils import is_version_compatible + +_ANCESTORS = { + "nautobot.extras.filters.NautobotFilterSet": ">=2", +} + +_VERSION_COMPATIBLE_ANCESTORS = [key for key, value in _ANCESTORS.items() if is_version_compatible(value)] + + +class NautobotSubClassNameChecker(BaseChecker): + """Ensure subclass name is . + + This can typically be done via .replace("Nautobot", ) + """ + + version_specifier = ">1,<3" + + name = "nautobot-sub-class-name" + msgs = { + "E4242": ( + "Sub-class name should be %s.", + "nb-sub-class-name", + "All classes should have a sub-class name that is .", + ) + } + + def visit_classdef(self, node: ClassDef): + """Visit class definitions.""" + ancestor = find_ancestor(node, _VERSION_COMPATIBLE_ANCESTORS) + if not ancestor: + return + + class_name = node.name + model_name = get_model_name(ancestor, node) + expected_name = ancestor.split(".")[-1].replace("Nautobot", model_name) + if expected_name != class_name: + self.add_message("nb-sub-class-name", node=node, args=(expected_name,)) diff --git a/pylint_nautobot/utils.py b/pylint_nautobot/utils.py index f4eb89e..f30da76 100644 --- a/pylint_nautobot/utils.py +++ b/pylint_nautobot/utils.py @@ -1,16 +1,74 @@ """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 Name from importlib_resources import files from packaging.specifiers import SpecifierSet from packaging.version import Version from yaml import safe_load +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_nautobot_v2_installed() -> bool: """Return True if Nautobot v2.x is installed.""" return MINIMUM_NAUTOBOT_VERSION.major == 2 diff --git a/tests/inputs/sub-class-name/error_filter_set.py b/tests/inputs/sub-class-name/error_filter_set.py new file mode 100644 index 0000000..d1fd241 --- /dev/null +++ b/tests/inputs/sub-class-name/error_filter_set.py @@ -0,0 +1,15 @@ +from nautobot.apps.filters import NautobotFilterSet +from nautobot.core.models.generics import PrimaryModel + + +class AddressObject(PrimaryModel): + pass + + +class MyAddressObjectFilterSet(NautobotFilterSet): + """Filter for AddressObject.""" + + class Meta: + """Meta attributes for filter.""" + + model = AddressObject diff --git a/tests/inputs/sub-class-name/good_filter_set.py b/tests/inputs/sub-class-name/good_filter_set.py new file mode 100644 index 0000000..0545356 --- /dev/null +++ b/tests/inputs/sub-class-name/good_filter_set.py @@ -0,0 +1,15 @@ +from nautobot.apps.filters import NautobotFilterSet +from nautobot.core.models.generics import PrimaryModel + + +class AddressObject(PrimaryModel): + pass + + +class AddressObjectFilterSet(NautobotFilterSet): + """Filter for AddressObject.""" + + class Meta: + """Meta attributes for filter.""" + + model = AddressObject diff --git a/tests/test_sub_class_name.py b/tests/test_sub_class_name.py new file mode 100644 index 0000000..c20d16d --- /dev/null +++ b/tests/test_sub_class_name.py @@ -0,0 +1,39 @@ +"""Tests for sub class name checker.""" + +from pylint.testutils import CheckerTestCase + +from pylint_nautobot.sub_class_name import NautobotSubClassNameChecker + +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_failing_node(module_node): + return module_node.body[3] + + +_EXPECTED_ERRORS = { + "filter_set": { + "msg_id": "nb-sub-class-name", + "line": 9, + "col_offset": 0, + "args": ("AddressObjectFilterSet",), + "node": _find_failing_node, + }, +} + + +class TestSubClassNameChecker(CheckerTestCase): + """Test sub class name checker""" + + CHECKER_CLASS = NautobotSubClassNameChecker + + @parametrize_error_files(__file__, _EXPECTED_ERRORS) + def test_sub_class_name(self, path, expected_error): + assert_error_file(self, path, expected_error) + + @parametrize_good_files(__file__) + def test_no_issues(self, path): + assert_good_file(self, path) From 33603b8e90d4ed4a34c8bb52d5ddd6cc7723ee42 Mon Sep 17 00:00:00 2001 From: Jan Snasel Date: Fri, 9 Feb 2024 13:24:24 +0000 Subject: [PATCH 4/7] chore: Fix test versions --- tests/test_sub_class_name.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_sub_class_name.py b/tests/test_sub_class_name.py index c20d16d..935358c 100644 --- a/tests/test_sub_class_name.py +++ b/tests/test_sub_class_name.py @@ -16,6 +16,7 @@ def _find_failing_node(module_node): _EXPECTED_ERRORS = { "filter_set": { + "version": ">=2", "msg_id": "nb-sub-class-name", "line": 9, "col_offset": 0, From a591a4d487289b4ab8d1672dff13ee9fe73cca73 Mon Sep 17 00:00:00 2001 From: Jan Snasel Date: Mon, 12 Feb 2024 08:35:39 +0000 Subject: [PATCH 5/7] fix: "version" => "versions" --- tests/test_sub_class_name.py | 2 +- tests/utils.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_sub_class_name.py b/tests/test_sub_class_name.py index 935358c..faaf194 100644 --- a/tests/test_sub_class_name.py +++ b/tests/test_sub_class_name.py @@ -16,7 +16,7 @@ def _find_failing_node(module_node): _EXPECTED_ERRORS = { "filter_set": { - "version": ">=2", + "versions": ">=2", "msg_id": "nb-sub-class-name", "line": 9, "col_offset": 0, diff --git a/tests/utils.py b/tests/utils.py index b1e3b56..8f11e10 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -24,8 +24,8 @@ def parametrize_error_files(module, expected_errors): def get_params(): for name in names: expected_error = {**expected_errors[name]} - version = expected_error.pop("version", "") - if is_version_compatible(version): + versions = expected_error.pop("versions", "") + if is_version_compatible(versions): path = inputs_path / f"error_{name}.py" yield path, expected_error From 8fb4bb0c42436aea220e32a47c10910f9f91165c Mon Sep 17 00:00:00 2001 From: Jan Snasel Date: Tue, 13 Feb 2024 10:51:52 +0000 Subject: [PATCH 6/7] feat: Sub Class Name classes and testing --- pylint_nautobot/sub_class_name.py | 68 +++++++++-- pylint_nautobot/utils.py | 112 +++++++++++++----- .../sub-class-name/error_filter_form.py | 12 ++ .../inputs/sub-class-name/error_model_form.py | 15 +++ .../inputs/sub-class-name/error_serializer.py | 15 +++ tests/inputs/sub-class-name/error_table.py | 15 +++ tests/inputs/sub-class-name/error_viewset.py | 12 ++ .../inputs/sub-class-name/good_filter_form.py | 12 ++ .../inputs/sub-class-name/good_model_form.py | 15 +++ .../inputs/sub-class-name/good_serializer.py | 15 +++ tests/inputs/sub-class-name/good_table.py | 15 +++ tests/inputs/sub-class-name/good_viewset.py | 12 ++ tests/test_sub_class_name.py | 40 +++++++ 13 files changed, 321 insertions(+), 37 deletions(-) create mode 100644 tests/inputs/sub-class-name/error_filter_form.py create mode 100644 tests/inputs/sub-class-name/error_model_form.py create mode 100644 tests/inputs/sub-class-name/error_serializer.py create mode 100644 tests/inputs/sub-class-name/error_table.py create mode 100644 tests/inputs/sub-class-name/error_viewset.py create mode 100644 tests/inputs/sub-class-name/good_filter_form.py create mode 100644 tests/inputs/sub-class-name/good_model_form.py create mode 100644 tests/inputs/sub-class-name/good_serializer.py create mode 100644 tests/inputs/sub-class-name/good_table.py create mode 100644 tests/inputs/sub-class-name/good_viewset.py diff --git a/pylint_nautobot/sub_class_name.py b/pylint_nautobot/sub_class_name.py index 648759e..577ab98 100644 --- a/pylint_nautobot/sub_class_name.py +++ b/pylint_nautobot/sub_class_name.py @@ -1,17 +1,58 @@ """Check for imports whose paths have changed in 2.0.""" +from typing import NamedTuple + from astroid import ClassDef from pylint.checkers import BaseChecker from .utils import find_ancestor from .utils import get_model_name +from .utils import is_abstract_class from .utils import is_version_compatible +from .utils import trim_first_pascal_word + +_ANCESTORS = ( + { + "versions": ">=2", + "ancestor": "nautobot.extras.filters.NautobotFilterSet", + }, + { + "versions": "<1", # Disabled, unable to find a model inside the class + "ancestor": "nautobot.extras.forms.base.BulkEditForm", + }, + { + "versions": ">=2", + "ancestor": "nautobot.extras.forms.base.NautobotFilterForm", + }, + { + "versions": ">=2", + "ancestor": "nautobot.extras.forms.base.NautobotModelForm", + "suffix": "Form", + }, + { + "versions": ">=2", + "ancestor": "nautobot.core.api.serializers.NautobotModelSerializer", + "suffix": "Serializer", + }, + { + "versions": ">=2", + "ancestor": "nautobot.core.views.viewsets.NautobotUIViewSet", + }, + { + "versions": ">=2", + "ancestor": "nautobot.core.tables.BaseTable", + }, +) + + +class _Ancestor(NamedTuple): + ancestor: str + suffix: str -_ANCESTORS = { - "nautobot.extras.filters.NautobotFilterSet": ">=2", -} -_VERSION_COMPATIBLE_ANCESTORS = [key for key, value in _ANCESTORS.items() if is_version_compatible(value)] +def _get_ancestor(item: dict) -> _Ancestor: + ancestor = item["ancestor"] + return _Ancestor(ancestor, item.get("suffix", trim_first_pascal_word(ancestor.split(".")[-1]))) class NautobotSubClassNameChecker(BaseChecker): @@ -20,7 +61,7 @@ class NautobotSubClassNameChecker(BaseChecker): This can typically be done via .replace("Nautobot", ) """ - version_specifier = ">1,<3" + version_specifier = ">=2,<3" name = "nautobot-sub-class-name" msgs = { @@ -31,14 +72,21 @@ class NautobotSubClassNameChecker(BaseChecker): ) } + def __init__(self, *args, **kwargs): + """Initialize the checker.""" + super().__init__(*args, **kwargs) + + self.ancestors = tuple(_get_ancestor(item) for item in _ANCESTORS if is_version_compatible(item["versions"])) + def visit_classdef(self, node: ClassDef): """Visit class definitions.""" - ancestor = find_ancestor(node, _VERSION_COMPATIBLE_ANCESTORS) + if is_abstract_class(node): + return + + ancestor = find_ancestor(node, self.ancestors, lambda item: item.ancestor) if not ancestor: return - class_name = node.name - model_name = get_model_name(ancestor, node) - expected_name = ancestor.split(".")[-1].replace("Nautobot", model_name) - if expected_name != class_name: + expected_name = get_model_name(node) + ancestor.suffix + if expected_name != node.name: self.add_message("nb-sub-class-name", node=node, args=(expected_name,)) diff --git a/pylint_nautobot/utils.py b/pylint_nautobot/utils.py index 2a4f911..be5e816 100644 --- a/pylint_nautobot/utils.py +++ b/pylint_nautobot/utils.py @@ -2,21 +2,27 @@ from importlib import metadata from pathlib import Path -from typing import List +from typing import Callable +from typing import Iterable from typing import Optional +from typing import TypeVar from typing import Union import toml from astroid import Assign from astroid import Attribute +from astroid import Call from astroid import ClassDef from astroid import Const from astroid import Name +from astroid import NodeNG from importlib_resources import files from packaging.specifiers import SpecifierSet from packaging.version import Version from yaml import safe_load +T = TypeVar("T") + def _read_poetry_lock() -> dict: for directory in (Path.cwd(), *Path.cwd().parents): @@ -42,6 +48,27 @@ def _read_locked_nautobot_version() -> Optional[str]: MINIMUM_NAUTOBOT_VERSION = Version(_read_locked_nautobot_version() or metadata.version("nautobot")) +def trim_first_pascal_word(pascal_case_string: str) -> str: + """Remove the first word from a pascal case string. + + Examples: + >>> trim_first_pascal_word("NautobotFilterSet") + 'FilterSet' + >>> trim_first_pascal_word("BaseTable") + 'Table' + >>> trim_first_pascal_word("FQDNModel") + 'Model' + """ + start_index = 0 + + for i in range(1, len(pascal_case_string)): + if pascal_case_string[i].isupper(): + start_index = i + break + + return pascal_case_string[start_index:] + + 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(): @@ -63,32 +90,63 @@ def is_abstract_class(node: ClassDef) -> bool: return False -def get_model_name(ancestor: str, node: ClassDef) -> str: +def find_attr(node: ClassDef, attr_name: str) -> Optional[Assign]: + """Get the attribute from the class definition.""" + for attr in node.body: + if isinstance(attr, Assign): + for target in attr.targets: + if ( + isinstance(target, (Name, Attribute)) + and getattr(target, "attrname", None) == attr_name + or getattr(target, "name", None) == attr_name + ): + return attr + return None + + +def find_meta(node: ClassDef) -> Optional[ClassDef]: + """Find the Meta class from the class definition.""" + for child in node.body: + if isinstance(child, ClassDef) and child.name == "Meta": + return child + return None + + +def get_model_name(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, - ) + queryset = find_attr(node, "queryset") + if queryset: + return find_model_name_from_queryset(queryset.value) + + model_attr = find_attr(node, "model") if not model_attr: - raise NotImplementedError("The Meta class does not define a model attribute.") + meta = find_meta(node) + if not meta: + raise NotImplementedError("This class does not have a Meta class.") + model_attr = find_attr(meta, "model") + if not model_attr: + raise NotImplementedError("The Meta class does not define a model attribute.") + + return get_model_name_from_attr(model_attr) + +def find_model_name_from_queryset(node: NodeNG) -> str: + """Get the model name from the queryset assignment value.""" + while node: + if isinstance(node, Call): + node = node.func + elif isinstance(node, Attribute): + if node.attrname == "objects" and isinstance(node.expr, Name): + return node.expr.name + node = node.expr + else: + break + + raise NotImplementedError("Model was not found") + + +def get_model_name_from_attr(model_attr: Assign) -> str: + """Get the model name from the model attribute.""" if isinstance(model_attr.value, Name): return model_attr.value.name if not isinstance(model_attr.value, Attribute): @@ -105,14 +163,14 @@ def get_model_name(ancestor: str, node: ClassDef) -> str: return model_attr_chain[-1] -def find_ancestor(node: ClassDef, ancestors: List[str]) -> str: +def find_ancestor(node: ClassDef, ancestors: Iterable[T], get_value: Callable[[T], str]) -> Optional[T]: """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: + if get_value(checked_ancestor) in ancestor_class_types: return checked_ancestor - return "" + return None def is_version_compatible(specifier_set: Union[str, SpecifierSet]) -> bool: diff --git a/tests/inputs/sub-class-name/error_filter_form.py b/tests/inputs/sub-class-name/error_filter_form.py new file mode 100644 index 0000000..6038a51 --- /dev/null +++ b/tests/inputs/sub-class-name/error_filter_form.py @@ -0,0 +1,12 @@ +from nautobot.core.models.generics import PrimaryModel +from nautobot.extras.forms import NautobotFilterForm + + +class AddressObject(PrimaryModel): + pass + + +class MyAddressObjectFilterForm(NautobotFilterForm): + """Filter for AddressObject.""" + + model = AddressObject diff --git a/tests/inputs/sub-class-name/error_model_form.py b/tests/inputs/sub-class-name/error_model_form.py new file mode 100644 index 0000000..4d3fe33 --- /dev/null +++ b/tests/inputs/sub-class-name/error_model_form.py @@ -0,0 +1,15 @@ +from nautobot.core.models.generics import PrimaryModel +from nautobot.extras.forms import NautobotModelForm + + +class AddressObject(PrimaryModel): + pass + + +class MyAddressObjectForm(NautobotModelForm): + """Filter for AddressObject.""" + + class Meta: + """Meta attributes.""" + + model = AddressObject diff --git a/tests/inputs/sub-class-name/error_serializer.py b/tests/inputs/sub-class-name/error_serializer.py new file mode 100644 index 0000000..5acf5f0 --- /dev/null +++ b/tests/inputs/sub-class-name/error_serializer.py @@ -0,0 +1,15 @@ +from nautobot.apps.api import NautobotModelSerializer +from nautobot.core.models.generics import PrimaryModel + + +class AddressObject(PrimaryModel): + pass + + +class MyAddressObjectSerializer(NautobotModelSerializer): + """Serializer for AddressObject.""" + + class Meta: + """Meta attributes for filter.""" + + model = AddressObject diff --git a/tests/inputs/sub-class-name/error_table.py b/tests/inputs/sub-class-name/error_table.py new file mode 100644 index 0000000..3590b9e --- /dev/null +++ b/tests/inputs/sub-class-name/error_table.py @@ -0,0 +1,15 @@ +from nautobot.apps.tables import BaseTable +from nautobot.core.models.generics import PrimaryModel + + +class AddressObject(PrimaryModel): + pass + + +class MyAddressObjectTable(BaseTable): + """Filter for AddressObject.""" + + class Meta: + """Meta attributes for filter.""" + + model = AddressObject diff --git a/tests/inputs/sub-class-name/error_viewset.py b/tests/inputs/sub-class-name/error_viewset.py new file mode 100644 index 0000000..8035c84 --- /dev/null +++ b/tests/inputs/sub-class-name/error_viewset.py @@ -0,0 +1,12 @@ +from nautobot.apps.views import NautobotUIViewSet +from nautobot.core.models.generics import PrimaryModel + + +class AddressObject(PrimaryModel): + pass + + +class MyAddressObjectUIViewSet(NautobotUIViewSet): + """Filter for AddressObject.""" + + queryset = AddressObject.objects.all() diff --git a/tests/inputs/sub-class-name/good_filter_form.py b/tests/inputs/sub-class-name/good_filter_form.py new file mode 100644 index 0000000..6d2ff22 --- /dev/null +++ b/tests/inputs/sub-class-name/good_filter_form.py @@ -0,0 +1,12 @@ +from nautobot.core.models.generics import PrimaryModel +from nautobot.extras.forms import NautobotFilterForm + + +class AddressObject(PrimaryModel): + pass + + +class AddressObjectFilterForm(NautobotFilterForm): + """Filter for AddressObject.""" + + model = AddressObject diff --git a/tests/inputs/sub-class-name/good_model_form.py b/tests/inputs/sub-class-name/good_model_form.py new file mode 100644 index 0000000..77dcdb7 --- /dev/null +++ b/tests/inputs/sub-class-name/good_model_form.py @@ -0,0 +1,15 @@ +from nautobot.core.models.generics import PrimaryModel +from nautobot.extras.forms import NautobotModelForm + + +class AddressObject(PrimaryModel): + pass + + +class AddressObjectForm(NautobotModelForm): + """Filter for AddressObject.""" + + class Meta: + """Meta attributes.""" + + model = AddressObject diff --git a/tests/inputs/sub-class-name/good_serializer.py b/tests/inputs/sub-class-name/good_serializer.py new file mode 100644 index 0000000..0e6bf27 --- /dev/null +++ b/tests/inputs/sub-class-name/good_serializer.py @@ -0,0 +1,15 @@ +from nautobot.apps.api import NautobotModelSerializer +from nautobot.core.models.generics import PrimaryModel + + +class AddressObject(PrimaryModel): + pass + + +class AddressObjectSerializer(NautobotModelSerializer): + """Serializer for AddressObject.""" + + class Meta: + """Meta attributes for filter.""" + + model = AddressObject diff --git a/tests/inputs/sub-class-name/good_table.py b/tests/inputs/sub-class-name/good_table.py new file mode 100644 index 0000000..928b817 --- /dev/null +++ b/tests/inputs/sub-class-name/good_table.py @@ -0,0 +1,15 @@ +from nautobot.apps.tables import BaseTable +from nautobot.core.models.generics import PrimaryModel + + +class AddressObject(PrimaryModel): + pass + + +class AddressObjectTable(BaseTable): + """Filter for AddressObject.""" + + class Meta: + """Meta attributes for filter.""" + + model = AddressObject diff --git a/tests/inputs/sub-class-name/good_viewset.py b/tests/inputs/sub-class-name/good_viewset.py new file mode 100644 index 0000000..ea9f39f --- /dev/null +++ b/tests/inputs/sub-class-name/good_viewset.py @@ -0,0 +1,12 @@ +from nautobot.apps.views import NautobotUIViewSet +from nautobot.core.models.generics import PrimaryModel + + +class AddressObject(PrimaryModel): + pass + + +class AddressObjectUIViewSet(NautobotUIViewSet): + """Filter for AddressObject.""" + + queryset = AddressObject.objects.all() diff --git a/tests/test_sub_class_name.py b/tests/test_sub_class_name.py index 966b3e1..3de290b 100644 --- a/tests/test_sub_class_name.py +++ b/tests/test_sub_class_name.py @@ -15,6 +15,14 @@ def _find_failing_node(module_node): _EXPECTED_ERRORS = { + "filter_form": { + "versions": ">=2", + "msg_id": "nb-sub-class-name", + "line": 9, + "col_offset": 0, + "args": ("AddressObjectFilterForm",), + "node": _find_failing_node, + }, "filter_set": { "versions": ">=2", "msg_id": "nb-sub-class-name", @@ -23,6 +31,38 @@ def _find_failing_node(module_node): "args": ("AddressObjectFilterSet",), "node": _find_failing_node, }, + "model_form": { + "versions": ">=2", + "msg_id": "nb-sub-class-name", + "line": 9, + "col_offset": 0, + "args": ("AddressObjectForm",), + "node": _find_failing_node, + }, + "serializer": { + "versions": ">=2", + "msg_id": "nb-sub-class-name", + "line": 9, + "col_offset": 0, + "args": ("AddressObjectSerializer",), + "node": _find_failing_node, + }, + "viewset": { + "versions": ">=2", + "msg_id": "nb-sub-class-name", + "line": 9, + "col_offset": 0, + "args": ("AddressObjectUIViewSet",), + "node": _find_failing_node, + }, + "table": { + "versions": ">=2", + "msg_id": "nb-sub-class-name", + "line": 9, + "col_offset": 0, + "args": ("AddressObjectTable",), + "node": _find_failing_node, + }, } From 8351a02dd856571a509b7b4002ffedb024130f73 Mon Sep 17 00:00:00 2001 From: Jan Snasel Date: Thu, 15 Feb 2024 10:50:42 +0000 Subject: [PATCH 7/7] fix: Use unique pylint ID --- pylint_nautobot/sub_class_name.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint_nautobot/sub_class_name.py b/pylint_nautobot/sub_class_name.py index 577ab98..68c3ffc 100644 --- a/pylint_nautobot/sub_class_name.py +++ b/pylint_nautobot/sub_class_name.py @@ -65,7 +65,7 @@ class NautobotSubClassNameChecker(BaseChecker): name = "nautobot-sub-class-name" msgs = { - "E4242": ( + "E4281": ( "Sub-class name should be %s.", "nb-sub-class-name", "All classes should have a sub-class name that is .",