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

Remove unecessary migrations blacklisting #293

Closed
wants to merge 3 commits into from

Conversation

carlio
Copy link
Collaborator

@carlio carlio commented Aug 28, 2020

From what I can see, the process is:

  1. pylint loads the pylint_django plugin and runs load_configuration (or, using compat, we do it) and we add migrations to the linter.config.blacklist : https://github.com/PyCQA/pylint-django/blob/7721691d73a26fea3d4c7e23b90d5cc8ad9c6f96/pylint_django/plugin.py#L13
  2. pylint_django loads its list of checkers including the migrations checker, which removess migrations from linter.config.blacklist : https://github.com/PyCQA/pylint-django/blob/7721691d73a26fea3d4c7e23b90d5cc8ad9c6f96/pylint_django/checkers/migrations.py#L159

So basically we add it to the blacklist then remove it almost immediately. I think from the comment that the migrations checker is trying to only remove it for its own checks, but the linter config is global so I believe that this code is unused and has no effect.

Since it probably also interferes with any user-provided configuration in their .pylintrc, and as far as I can tell it hasn't caused any problems that everything checks migrations folders, I think we should remove the code to clean up as I think it's essentially a no-op

…the plugin is loaded, migrations are added to the blacklist. The migrations checker then removes them from the blacklist, and since it edits linter.config, it is not only done for the migrations checker but globally. Therefore the plugin should simply not manage the blacklist for migrations (also avoiding overriding any user-specified behaviour in their .pylintrc)
@coveralls
Copy link

coveralls commented Aug 28, 2020

Pull Request Test Coverage Report for Build 1252

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 86.967%

Totals Coverage Status
Change from base Build 1234: -0.02%
Covered Lines: 714
Relevant Lines: 821

💛 - Coveralls

@atodorov
Copy link
Contributor

atodorov commented Aug 29, 2020

So basically we add it to the blacklist then remove it almost immediately. I think from the comment that the migrations checker is trying to only remove it for its own checks, but the linter config is global so I believe that this code is unused and has no effect.

Migrations checker is not enabled by default. You need to --load-plugins=pylint_django.checkers.migrations.

My vote goes against this change. If someone enables both pylint_django and the migrations checker then they end up without the black list anyway.

@carlio
Copy link
Collaborator Author

carlio commented Aug 30, 2020

@atodorov Ah I didn't know that the migrations was a separate plugin, hence my confusion. In that case I agree, this PR is unecessary as the behaviour is currently correct.

@carlio carlio closed this Aug 30, 2020
@carlio carlio deleted the 289-remove-repeated-migration-config branch January 2, 2022 14:17
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.

None yet

3 participants