Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve incorrect base class checker #72

Merged
merged 5 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 57 additions & 19 deletions pylint_nautobot/incorrect_base_class.py
Original file line number Diff line number Diff line change
@@ -1,31 +1,79 @@
"""Check for imports whose paths have changed in 2.0."""
"""Check for incorrect base classes."""
from typing import NamedTuple

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 .utils import is_version_compatible


def is_abstract(node):
def is_abstract(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"
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


_CLASS_MAPPING = (
cmsirbu marked this conversation as resolved.
Show resolved Hide resolved
{
"incorrect": "django_filters.filters.FilterSet",
"correct": "django_filters.filters.BaseFilterSet",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"correct": "django_filters.filters.BaseFilterSet",
"correct": "nautobot.apps.filters.NautobotFilterSet",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

},
{
"incorrect": "django.db.models.base.Model",
"correct": "nautobot.core.models.BaseModel",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we support OR but Primary should always be the preferred and base is only used for custom through tables and a few other scenarios.

Suggested change
"correct": "nautobot.core.models.BaseModel",
"correct": ["nautobot.apps.models.PrimaryModel","nautobot.apps.models.BaseModel"]

Copy link
Contributor Author

@snaselj snaselj Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PrimaryModel is a BaseModel descendant. If we want to allow BaseModel to be used, it covers PrimaryModel as well.

One simple option is to enforce PrimaryModel, instead of BaseModel. In such case, using BaseModel only would have to be disabled by # pylint: disable=nb-incorrect-base-class macro.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote enforce PrimaryModel

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PrimaryModel is enforced now.

},
{
"versions": "<2.0",
"incorrect": "django.forms.forms.Form",
"correct": "nautobot.utilities.forms.forms.BootstrapMixin",
},
{
"versions": ">=2.0",
"incorrect": "django.forms.forms.Form",
"correct": "nautobot.core.forms.forms.BootstrapMixin",
},
{
"versions": ">=2.0",
"incorrect": "django.forms.ModelForm",
"correct": "nautobot.apps.forms.NautobotModelForm",
},
{
"versions": ">=2.0",
"incorrect": "nautobot.extras.plugins.PluginConfig",
"correct": "nautobot.apps.NautobotAppConfig",
},
)


class _Mapping(NamedTuple):
incorrect: str
correct: str


_COMPATIBLE_MAPPING = (
_Mapping(item["incorrect"], item["correct"])
for item in _CLASS_MAPPING
if is_version_compatible(item.get("versions", ""))
)


class NautobotIncorrectBaseClassChecker(BaseChecker):
"""Check that all X inherits from Y.

Expand All @@ -36,17 +84,6 @@ class NautobotIncorrectBaseClassChecker(BaseChecker):

# Maps a non-Nautobot-specific base class to a Nautobot-specific base class which has to be in the class hierarchy
# for every class that has the base class in its hierarchy.
external_to_nautobot_class_mapping = [
("django_filters.filters.FilterSet", "django_filters.filters.BaseFilterSet"),
("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",
),
]

name = "nautobot-incorrect-base-class"
msgs = {
"E4242": (
Expand All @@ -56,7 +93,7 @@ class NautobotIncorrectBaseClassChecker(BaseChecker):
)
}

def visit_classdef(self, node):
def visit_classdef(self, node: ClassDef):
"""Visit class definitions."""
if is_abstract(node):
return
Expand All @@ -66,6 +103,7 @@ def visit_classdef(self, node):
return

ancestor_class_types = [ancestor.qname() for ancestor in node.ancestors()]
for base_class, nautobot_base_class in self.external_to_nautobot_class_mapping:
if base_class in ancestor_class_types and nautobot_base_class not in ancestor_class_types:
self.add_message(msgid="nb-incorrect-base-class", node=node, args=(base_class, nautobot_base_class))
for mapping in _COMPATIBLE_MAPPING:
if mapping.incorrect in ancestor_class_types and mapping.correct not in ancestor_class_types:
self.add_message(msgid="nb-incorrect-base-class", node=node, args=(mapping.incorrect, mapping.correct))
return
5 changes: 0 additions & 5 deletions pylint_nautobot/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@
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):
Expand Down