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

Implement a checker to validate that q uses SearchFIlter #106

Closed
wants to merge 12 commits into from

Conversation

smk4664
Copy link
Contributor

@smk4664 smk4664 commented Dec 31, 2024

Closes #99

This PR implements a new checker to validate that Filtersets use SearchFilter for the q search instead of django_filters.CharField with a custom search method.

@smk4664 smk4664 requested review from cmsirbu, Kircheneer and a team as code owners December 31, 2024 23:07
@smk4664 smk4664 self-assigned this Dec 31, 2024
pylint_nautobot/q_search_filter.py Show resolved Hide resolved
pylint_nautobot/q_search_filter.py Outdated Show resolved Hide resolved
pylint_nautobot/q_search_filter.py Outdated Show resolved Hide resolved
pylint_nautobot/q_search_filter.py Outdated Show resolved Hide resolved
pylint_nautobot/q_search_filter.py Outdated Show resolved Hide resolved
docs/development.md Outdated Show resolved Hide resolved
docs/development.md Outdated Show resolved Hide resolved
pylint_nautobot/constants.py Outdated Show resolved Hide resolved
pylint_nautobot/__init__.py Outdated Show resolved Hide resolved
pylint_nautobot/constants.py Outdated Show resolved Hide resolved
pylint_nautobot/constants.py Show resolved Hide resolved
pylint_nautobot/q_search_filter.py Show resolved Hide resolved
from pylint.typing import MessageDefinitionTuple


class MESSAGES: # pylint: disable=too-few-public-methods
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there prior art for this in pylint or other pylint projects? I think we should try to be consistent with the parent library and their documentation in cases like pylint where the learning curve is already a cliff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar in their examples and their codebase. The difference is that they use multiple codes in a single class attribute, and they inherit from BaseChecker, but also combine some utils. I will post their code. I decided on using a separate attribute for each code to help solve the issue of finding all the codes used.

Here is an example, perhaps I could change this to MESSAGESMixin:

https://github.com/pylint-dev/pylint/blob/main/pylint/checkers/deprecated.py

Copy link
Contributor

Choose a reason for hiding this comment

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

We decided after discussion with @cmsirbu and @glennmatthews that @smk4664 will have to do the work to revert these changes and we'll get a follow-up issue opened to introduce an invoke command to list out the current pylint message codes (pylint --list-msgs-enabled | grep 'nb-')

@smk4664
Copy link
Contributor Author

smk4664 commented Jan 10, 2025

Closing, I will reopen with the rules, but following the latest comments from Gary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a new linter to ensure filterset q filters are using SearchFilter
5 participants