FFI-8 P4: fix pylint failures #608
Conversation
mariajgrimaldi
changed the base branch from
master
to
edunext/limonero.master
mariajgrimaldi
requested review from
JuanDavidBuitrago,
MaferMazu,
MoisesGSalas,
black-redoc and
johanseto
Fix pylint violation in lms, cms, openedx and common applications.
mariajgrimaldi
force-pushed
the
li/ednx/FFI-8_p4
branch
from
1ab565f to
ee7f33f
Compare
| from django.http import HttpResponse | ||
| from django.utils.decorators import method_decorator | ||
| from django.views.generic import View | ||
| from organizations.api import get_organizations |
There was a problem hiding this comment.
We didn't need this method after this PR: #574
| username=self.user.username, | ||
| email=self.user.email, | ||
| name=self.user.profile.name, | ||
| name=self.user.profile.name, # pylint: disable=no-member |
There was a problem hiding this comment.
We didn't disable this violation in this PR: #567
| from openedx_events.tests.utils import OpenEdxEventsTestMixin | ||
| from openedx.core.djangolib.testing.utils import skip_unless_lms | ||
|
|
||
| from common.djangoapps.student.models import CourseEnrollmentAllowed, CourseEnrollment |
There was a problem hiding this comment.
We didn't fix those imports in this PR: #567
| from django.test import override_settings | ||
| from openedx_filters.learning.enrollment import PreEnrollmentFilter | ||
| from openedx_filters import PipelineStep | ||
|
|
There was a problem hiding this comment.
We didn't fix these imports in this PR: #573
| from opaque_keys.edx.django.models import CourseKeyField | ||
| from simple_history.models import HistoricalRecords | ||
| from openedx_events.learning.data import CourseData, UserData, UserPersonalData, CertificateData | ||
| from openedx_events.learning.signals import CERTIFICATE_CHANGED, CERTIFICATE_CREATED, CERTIFICATE_REVOKED |
There was a problem hiding this comment.
We didn't fix those imports in this PR: #567
|
|
||
| from unittest import mock | ||
|
|
||
| from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user |
There was a problem hiding this comment.
We didn't remove this unused import in this PR: #566
| from django.utils.encoding import python_2_unicode_compatible | ||
| from opaque_keys.edx.django.models import CourseKeyField | ||
| from openedx_events.learning.data import CohortData, CourseData, UserData, UserPersonalData | ||
| from openedx_events.learning.signals import COHORT_MEMBERSHIP_CHANGED |
There was a problem hiding this comment.
We didn't fix those imports in this PR: #567
| from openedx.core.djangolib.testing.utils import skip_unless_lms | ||
|
|
||
| from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory | ||
|
|
There was a problem hiding this comment.
We didn't fix those imports in this PR: #567
| request.expires_in = getattr(settings, 'CLIENT_CREDENTIALS_ACCESS_TOKEN_EXPIRE_SECONDS', 31557600) | ||
|
|
||
| super(EdxOAuth2Validator, self).save_bearer_token(token, request, *args, **kwargs) | ||
| super().save_bearer_token(token, request, *args, **kwargs) |
There was a problem hiding this comment.
We didn't modernize this super here: #595
| from freezegun import freeze_time | ||
| from common.djangoapps.student.tests.factories import RegistrationFactory, UserFactory, UserProfileFactory | ||
| from openedx_events.tests.utils import OpenEdxEventsTestMixin | ||
| from common.djangoapps.student.tests.factories import RegistrationFactory, UserFactory, UserProfileFactory |
There was a problem hiding this comment.
We didn't fix those imports in this PR: #567
| username=user.username, | ||
| email=user.email, | ||
| name=user.profile.name, | ||
| name=user.profile.name, # pylint: disable=no-member |
There was a problem hiding this comment.
We didn't disable this violation here: #567
There was a problem hiding this comment.
Awesome, this PR fixes the old PRs we passed and affects the pylint performance (Also some mentioned in the conversation). I think this solves the 4 tests missing in #606. The checks seem to be all working since this PR.
Description
This PR fixes some pylint violations made during the course of the lilac migration. They didn't appear on the error logs because of a github actions configuration explained better here: #606
Some context
The platform at the lilac cut had a few quality errors that in edx-platform@master were already solved. So then, we had two options, backport those fixes -a big no, more changes to support- or ignore them completely and fix new violations introduced by our changes. To ignore them, we counted the total of pylint violations in the lilac release and used the number as our threshold. So, for example, if the admitted pylint violations for the common app were 30, then when adding a change that introduces another violation, pylint checks will fail. That's the point.
Unfortunately, for the reason explained above, we missed approx. 15 violations -IMPORTANT: These were introduced by our changes- which we're fixing with this PR.
For the next migration: the ideal situation is fixing these style issues when introducing the changes, not after.