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

Fix prospector dependencies #4149

Merged
merged 4 commits into from May 29, 2018
Merged

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented May 25, 2018

Due PyCQA/prospector@f866efc#diff-2eeaed663bd0d25b7e608891384b7298 pylint-django and pylint-celery aren't installed by default.

There isn't a changelog for the 0.12.9 version but it was released on pypi :/

@stsewd
Copy link
Member Author

@stsewd stsewd commented May 25, 2018

There are new rules that we need to fix

Loading

@stsewd stsewd force-pushed the fix-prospector-deps branch from f56ef2c to 79dfee4 May 25, 2018
@stsewd stsewd requested a review from May 25, 2018
pylint-django
pylint-celery
prospector
Copy link
Contributor

@davidfischer davidfischer May 25, 2018

Choose a reason for hiding this comment

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

Do you think we should pin some of these? At least to major versions?

Loading

Copy link
Member Author

@stsewd stsewd May 25, 2018

Choose a reason for hiding this comment

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

I think yes, but we'll need to pin minor versions. prospector breaking change was from 0.12.8 -> 0.12.9 :/

Loading

Copy link
Member

@ericholscher ericholscher May 29, 2018

Choose a reason for hiding this comment

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

Yea, our linting seems to break on a regular basis. Is there an argument for not just pinning them to a specific release? I don't feel like linting is something we need new features from very often.

Loading

Copy link
Member Author

@stsewd stsewd May 29, 2018

Choose a reason for hiding this comment

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

The only downside I see if that we'll need to update them manually. Should I pin them in this PR?

Loading

Copy link
Member

@ericholscher ericholscher May 29, 2018

Choose a reason for hiding this comment

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

Yea -- I think "pin them and update them manually and confirm they don't break" is better than "randomly breaks and we have a bunch of failing PR's where we rush to fix it"

Loading

prospector
pylint-django
pyflakes
maxcdn==0.0.7
Copy link
Member Author

@stsewd stsewd May 29, 2018

Choose a reason for hiding this comment

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

Thi dependency is also on deploy.txt, and if it's deleted all tests pass, not sure if we really need this here.

Loading

Copy link
Member

@ericholscher ericholscher May 29, 2018

Choose a reason for hiding this comment

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

Yea, we aren't supporting maxcdn anymore, but we might have tests for it. This is related to #2091

Loading

maxcdn
# astroid 1.6.2 breaks pylint-django
# https://github.com/PyCQA/pylint-django/issues/117
astroid==1.6.1
Copy link
Member Author

@stsewd stsewd May 29, 2018

Choose a reason for hiding this comment

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

Looks like the astroid bug was fixed on a new release :)

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented May 29, 2018

I pin all the lint deps to their latest stable release

Loading

@ericholscher ericholscher merged commit c6ce437 into readthedocs:master May 29, 2018
1 check passed
Loading
@ericholscher
Copy link
Member

@ericholscher ericholscher commented May 29, 2018

👍

Loading

@stsewd stsewd deleted the fix-prospector-deps branch May 29, 2018
@ericholscher ericholscher mentioned this pull request Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants