-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
There are new rules that we need to fix |
f56ef2c
to
79dfee4
Compare
requirements/lint.txt
Outdated
pylint-django | ||
pylint-celery | ||
prospector |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
prospector | ||
pylint-django | ||
pyflakes | ||
maxcdn==0.0.7 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
maxcdn | ||
# astroid 1.6.2 breaks pylint-django | ||
# https://github.com/PyCQA/pylint-django/issues/117 | ||
astroid==1.6.1 |
There was a problem hiding this comment.
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 :)
I pin all the lint deps to their latest stable release |
👍 |
Due prospector-dev/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 :/