-
Notifications
You must be signed in to change notification settings - Fork 47
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
chore: Upgrade Requirements #181
Conversation
Thanks for the pull request, @Ali-Salman29! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
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.
Thanks for this PR @Ali-Salman29 ! Please see comments.
- Configured django-settings-module to point to the project settings for proper integration with pylint-django. - Addressed compatibility with the newer pylint version, which enforces a limit of 5 positional arguments by default. Added too-many-positional-arguments to the ignore list to accommodate functions exceeding this limit.
4a8013b
to
a5a4aac
Compare
@pomegranited thank you for the review. I've updated the dependencies; please review it again. |
@pomegranited, can you please review it? |
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.
👍 one nit about the pylintrc
changes still being there, but the rest looks great, thank you @Ali-Salman29 !
-
I tested thisgreen CI is sufficient - Bumps this package's version
- I read through the code and checked the dependencies updated for breaking changes.
# ------------------------------ | ||
[MASTER] | ||
ignore = | ||
persistent = yes | ||
load-plugins = edx_lint.pylint,pylint_django,pylint_celery | ||
django-settings-module = edxsearch.settings |
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.
Can these changes to pylintrc
be reverted?
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.
For local updates, we need to update pylint_tweaks and then run edx_lint write pylintrc
to update the pylintrc file. Previously, I updated the file manually, but now it is updated automatically by the command.
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.
Ok cool, thanks @Ali-Salman29 , I didn't know how that file was maintained :)
Good to merge 👍
This PR improves the project's linting configuration and updates dependencies: