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

Use pylint #3727

Merged
merged 8 commits into from May 8, 2020
Merged

Use pylint #3727

merged 8 commits into from May 8, 2020

Conversation

Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Apr 5, 2019

Has pytest run pylint on Python files.

pylintrc is currently skipping all tests that affect the current codebase. Later changes may solve one or more of the corresponding issues and remove them from the list of ignored issues.

@Gallaecio Gallaecio force-pushed the pylint branch 2 times, most recently from 9f01471 to 4a95e09 Compare Apr 11, 2019
@codecov
Copy link

codecov bot commented Apr 11, 2019

Codecov Report

Merging #3727 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3727      +/-   ##
==========================================
- Coverage   84.55%   84.50%   -0.05%     
==========================================
  Files         164      164              
  Lines        9898     9903       +5     
  Branches     1480     1481       +1     
==========================================
  Hits         8369     8369              
- Misses       1261     1266       +5     
  Partials      268      268              
Impacted Files Coverage Δ
scrapy/pipelines/files.py 59.93% <0.00%> (-1.00%) ⬇️
scrapy/core/engine.py 87.87% <0.00%> (ø)
scrapy/utils/request.py 100.00% <0.00%> (ø)
scrapy/pipelines/media.py 97.29% <0.00%> (ø)
scrapy/utils/deprecate.py 95.38% <0.00%> (ø)
scrapy/http/request/form.py 95.34% <0.00%> (ø)
scrapy/core/downloader/handlers/ftp.py 98.36% <0.00%> (ø)

@dangra dangra changed the title Use pytest-pylint [MRG+1] Use pytest-pylint Jun 13, 2019
@dangra
Copy link
Member

dangra commented Jun 13, 2019

pylintrc is currently skipping all tests that affect the current codebase. Later changes may solve one or more of the corresponding issues and remove them from the list of ignored issues.

I like the approach although I'd prefer to be able to disable checks only for the files that are not passing them. It was part of the approach we took with tests to port the codebase to Python3.

I understand my suggestion may not be possible for lint errors or too many exceptions to maintain

@Gallaecio
Copy link
Member Author

Gallaecio commented Jun 14, 2019

I’ll work on it.

@Gallaecio Gallaecio changed the title [MRG+1] Use pytest-pylint [WIP] Use pytest-pylint Jun 14, 2019
@Gallaecio Gallaecio force-pushed the pylint branch 2 times, most recently from efe15e3 to 5586b7c Compare Jun 18, 2019
@Gallaecio Gallaecio changed the title [WIP] Use pytest-pylint Use pytest-pylint Jun 19, 2019
@Gallaecio
Copy link
Member Author

Gallaecio commented Jun 19, 2019

Done.

The only exception is a rule that is specific to Python 3 (not subclassing object) and should be only applied once Python 2 support is dropped.

@Gallaecio Gallaecio changed the title Use pytest-pylint [WIP] Use pytest-pylint Jun 24, 2019
@Gallaecio Gallaecio force-pushed the pylint branch 4 times, most recently from 1f31771 to 69876f1 Compare Jun 24, 2019
@Gallaecio
Copy link
Member Author

Gallaecio commented Jun 25, 2019

I’ve configured pytest-pylint to run on Python 3 only. This is because Pylint dropped support for Python 2 a while ago, and most of the issues found on Python 2 were due to Pylint bugs that have been already fixed but will never be backported for Python 2.

@Gallaecio Gallaecio changed the title [WIP] Use pytest-pylint Use pytest-pylint Jun 25, 2019
@Gallaecio Gallaecio changed the title Use pytest-pylint Use pylint Aug 12, 2019
@Gallaecio
Copy link
Member Author

Gallaecio commented Aug 12, 2019

Updated to resolve conflicts and run in its own continuous integration worker, instead of using pytest-pylint to run in all workers, in line with #3945

@Gallaecio
Copy link
Member Author

Gallaecio commented Oct 22, 2019

Updated to skip checks globally again. Unlike flake8, pylint does not support ignoring a list of files specifying the checks to ignore per file.

And because most changes will be to existing files, and most files do not pass one of the checks, excluding files instead of checks would not cover many files.

So, the plan is to ignore specific checks globally, and fix checks one by one in later pull requests.

@kmike kmike merged commit 794ab19 into scrapy:master May 8, 2020
2 checks passed
@kmike
Copy link
Member

kmike commented May 8, 2020

thanks @Gallaecio!

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