fix: bump pylint version #31084

UsamaSadiq · 2022-10-04T07:03:53Z

Description

Remove the pylint constraint and bumped to the latest version.
Updated the pylintrc_tweaks and pylintrc to include new pylint warnings.
Fixed quality failures for new pylint version.

Major Change

Dropped the pylint-django-settings plugin in pylintrc and instead provided settings path directly in the pylint shards in CI.
Latest changes in the pylint's internal parser and linter broke the custom script which will be fixed later on.

Follow Up issues

UsamaSadiq · 2022-10-04T16:09:07Z

Hi @nedbat
We'd previously added custom pylint-django-settings plugin to provide settings path when running pylint from local.
With the latest pylint parser changes, the script started failing so I've excluded the plug-in for now to remove the blocker for the latest pylint version.
For now, when running pylint from local, settings path would be needed to passed manually.
Should we remove the script altogether and add instructions on how to run pylint from local? Meanwhile, after merging this PR, arbi-bom will try to fix the failure in the plugin.

cms/djangoapps/contentstore/views/course.py

common/djangoapps/student/tests/test_email.py

nedbat

Two of the warning suppressions should be removed, and the code fixed instead.

nedbat · 2022-10-05T16:51:32Z

@UsamaSadiq The pylint-django-settings.py plugin seems useful, so let's not remove it. If you will be fixing the plugin soon, then maybe an announcement in the #dev channel is enough? I will guess that most devs don't try to run pylint locally anyway.

ashultz0 · 2022-10-12T19:39:44Z

I was actually fixing this in another repo edx/portal-designer#195 and determined that we don't put the django settings check in from the cookie cutter these days so I took it out - if we think it's good it probably should also be in the cookie cutter

bradenmacdonald · 2022-10-12T20:57:06Z

Re your issues with the Django settings path, I had what may be a similar issue in openedx-unsupported/blockstore#208 and I fixed it with this:

https://github.com/openedx/blockstore/blob/c2a782b5ce7e6b3e866f537521282a545e6d597e/pylintrc_tweaks#L3-L5

Not sure if that'll be useful here but just letting you know in case it is.

UsamaSadiq · 2022-10-17T11:19:32Z

xmodule/capa/correctmap.py

@@ -91,7 +91,7 @@ def set_dict(self, correct_map):

 """
 # empty current dict
- self.__init__()
+ self.__init__() # pylint: disable=unnecessary-dunder-call


Changing the __init__() call with Class declaration and assigning it to self as self=CorrectMap() causes self-cls-assignment warning to be generated.
Couldn't figure out what should be the way to go about this scenario.

UsamaSadiq · 2022-10-17T13:59:57Z

xmodule/modulestore/tests/django_utils.py

@@ -318,7 +318,7 @@ def start_modulestore_isolation(cls):

 cls.__old_modulestores.append(copy.deepcopy(settings.MODULESTORE))
 cls.__old_contentstores.append(copy.deepcopy(settings.CONTENTSTORE))
- override.__enter__()
+ override.__enter__() # pylint: disable=unnecessary-dunder-call


Refactoring to use ContextManager with instead as with open(override_settings(...)) as override causes the unit tests to fail with the error TypeError: expected str, bytes or os.PathLike object, not override_settings.
Detailed Error Stack Trace: https://github.com/openedx/edx-platform/actions/runs/3265468332/jobs/5367724316#step:7:3517

UsamaSadiq · 2022-10-17T14:00:53Z

openedx/core/djangolib/testing/utils.py

@@ -101,7 +101,7 @@ def start_cache_isolation(cls):

 cls.__old_settings.append(copy.deepcopy(settings.CACHES))
 override = override_settings(CACHES=cache_settings)
- override.__enter__()
+ override.__enter__() # pylint: disable=unnecessary-dunder-call


This also causes the unit tests to fail upon refactoring. Detailed Error Stack Trace: https://github.com/openedx/edx-platform/actions/runs/3265468332/jobs/5367724316#step:7:3517

UsamaSadiq · 2022-10-17T14:03:10Z

pylintrc

+ missing-timeout,
+ c-extension-no-member,
+ no-name-in-module,
+ unnecessary-lambda-assignment,


Fixed some of the lambda assignment warnings. There are still more than ~90 warnings remaining. Fixing all the warnings in the same PR will make the review a lot more difficult so disabling the warning for now so it can be enabled and fixed in a later PR.
The warnings report shows following no of warnings after enabling it:

lms-2: 27

openedx-1: 6

common: 6

cms: 11

xmodule: 46

Created a follow up issue #31199 for this task.

Why are we manually editing the pylintrc file and not changing the tweeks file...?

The updated pylintrc file has been generated by the updated pylintrc_tweaks file. You can see the detailed changes in the pylintrc_tweaks file the changes section.

fix: drop pylint_django_settings plugin

UsamaSadiq · 2022-10-17T14:15:39Z

xmodule/x_module.py

@@ -1844,7 +1846,7 @@ def render(self, block, view_name, context=None):

 """
 context = context or {}
- return self.__getattr__('render')(block, view_name, context)
+ return self.__getattr__('render')(block, view_name, context) # pylint: disable=unnecessary-dunder-call


Refactoring self.__getattr__('render') to getattr(self, 'render') generates C7630: literal-used-as-attribute warning and using self.render instead causes the tests to go into infinite recursion.

aht007

Rest of the PR looks good except the above question about pylintrc

edx-pipeline-bot · 2022-10-27T08:24:13Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2022-10-27T08:52:17Z

EdX Release Notice: This PR has been deployed to the production environment.

UsamaSadiq force-pushed the usamasadiq/bump-pylint-version branch 3 times, most recently from 63734d1 to 8d57cbf Compare October 4, 2022 12:08

UsamaSadiq marked this pull request as ready for review October 4, 2022 15:40

nedbat reviewed Oct 5, 2022

View reviewed changes

cms/djangoapps/contentstore/views/course.py Outdated Show resolved Hide resolved

nedbat reviewed Oct 5, 2022

View reviewed changes

cms/djangoapps/contentstore/views/course.py Outdated Show resolved Hide resolved

nedbat reviewed Oct 5, 2022

View reviewed changes

common/djangoapps/student/tests/test_email.py Outdated Show resolved Hide resolved

nedbat suggested changes Oct 5, 2022

View reviewed changes

UsamaSadiq commented Oct 17, 2022

View reviewed changes

UsamaSadiq force-pushed the usamasadiq/bump-pylint-version branch 3 times, most recently from 0f35b96 to dc5c203 Compare October 17, 2022 13:57

UsamaSadiq commented Oct 17, 2022

View reviewed changes

fix: bump pylint version

580230c

fix: drop pylint_django_settings plugin

UsamaSadiq force-pushed the usamasadiq/bump-pylint-version branch from dc5c203 to 580230c Compare October 17, 2022 14:12

UsamaSadiq commented Oct 17, 2022

View reviewed changes

UsamaSadiq requested review from nedbat and aht007 October 17, 2022 14:19

This was referenced Oct 25, 2022

Fix pylint-django-settings plugin #31198

Open

Fix all the occurrences of unnecessary-lambda-assignment warning #31199

Open

aht007 reviewed Oct 25, 2022

View reviewed changes

UsamaSadiq requested a review from aht007 October 26, 2022 04:56

aht007 approved these changes Oct 26, 2022

View reviewed changes

nedbat approved these changes Oct 26, 2022

View reviewed changes

Merge branch 'master' into usamasadiq/bump-pylint-version

ec3180c

UsamaSadiq merged commit 4734f9f into master Oct 27, 2022

UsamaSadiq deleted the usamasadiq/bump-pylint-version branch October 27, 2022 07:19

jcohen28 pushed a commit to Medality-Health/edx-platform that referenced this pull request Nov 4, 2022

fix: bump pylint version (openedx#31084)

f6a8d76

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bump pylint version #31084

fix: bump pylint version #31084

UsamaSadiq commented Oct 4, 2022 •

edited

Loading

UsamaSadiq commented Oct 4, 2022

nedbat left a comment

nedbat commented Oct 5, 2022

ashultz0 commented Oct 12, 2022

bradenmacdonald commented Oct 12, 2022

UsamaSadiq Oct 17, 2022

UsamaSadiq Oct 17, 2022

UsamaSadiq Oct 17, 2022

UsamaSadiq Oct 17, 2022 •

edited

Loading

UsamaSadiq Oct 25, 2022

aht007 Oct 25, 2022

UsamaSadiq Oct 25, 2022

UsamaSadiq Oct 17, 2022

aht007 left a comment

edx-pipeline-bot commented Oct 27, 2022

edx-pipeline-bot commented Oct 27, 2022

fix: bump pylint version #31084

fix: bump pylint version #31084

Conversation

UsamaSadiq commented Oct 4, 2022 • edited Loading

Description

Major Change

Follow Up issues

UsamaSadiq commented Oct 4, 2022

nedbat left a comment

Choose a reason for hiding this comment

nedbat commented Oct 5, 2022

ashultz0 commented Oct 12, 2022

bradenmacdonald commented Oct 12, 2022

UsamaSadiq Oct 17, 2022

Choose a reason for hiding this comment

UsamaSadiq Oct 17, 2022

Choose a reason for hiding this comment

UsamaSadiq Oct 17, 2022

Choose a reason for hiding this comment

UsamaSadiq Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

UsamaSadiq Oct 25, 2022

Choose a reason for hiding this comment

aht007 Oct 25, 2022

Choose a reason for hiding this comment

UsamaSadiq Oct 25, 2022

Choose a reason for hiding this comment

UsamaSadiq Oct 17, 2022

Choose a reason for hiding this comment

aht007 left a comment

Choose a reason for hiding this comment

edx-pipeline-bot commented Oct 27, 2022

edx-pipeline-bot commented Oct 27, 2022

UsamaSadiq commented Oct 4, 2022 •

edited

Loading

UsamaSadiq Oct 17, 2022 •

edited

Loading