-
Notifications
You must be signed in to change notification settings - Fork 2
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
Python upgrade from 3.9.x to 3.12.x #3089
Changes from all commits
358a3a9
28a60a5
aa4a660
314d076
334bf84
deff5a9
b447d54
e3dffde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
FROM python:3.9.15 | ||
FROM python:3.12.4 | ||
LABEL maintainer "ODL DevOps <[email protected]>" | ||
|
||
# Add package files, install updated node and pip | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,14 +33,15 @@ def mock_cybersource_wsdl(mocked_responses, settings, service_version=SERVICE_VE | |
Mocks the responses to achieve a functional WSDL | ||
""" | ||
# in order for zeep to load the wsdl, it will load the wsdl and the accompanying xsd definitions | ||
with open(f"{DATA_DIR}/CyberSourceTransaction_{service_version}.wsdl", "r") as wsdl: # noqa: PTH123, UP015 | ||
# Note: open() defaults to read mode ("r") | ||
with open(f"{DATA_DIR}/CyberSourceTransaction_{service_version}.wsdl") as wsdl: # noqa: PTH123 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The file open is readable by default but explicitly adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not complain but the
Note: The first option will be applied consistently to all instances where this change is present and/or explicitly mentioned. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the details you answered my question but maybe inverted, below is why.
I think if pyupgrade complains about it that should mean that the python upgrade does complain. In, which case maybe we can remove the |
||
mocked_responses.add( | ||
mocked_responses.GET, | ||
settings.CYBERSOURCE_WSDL_URL, | ||
body=wsdl.read(), | ||
status=status.HTTP_200_OK, | ||
) | ||
with open(f"{DATA_DIR}/CyberSourceTransaction_{SERVICE_VERSION}.xsd", "r") as xsd: # noqa: PTH123, UP015 | ||
with open(f"{DATA_DIR}/CyberSourceTransaction_{SERVICE_VERSION}.xsd") as xsd: # noqa: PTH123 | ||
mocked_responses.add( | ||
mocked_responses.GET, | ||
f"http://localhost/service/CyberSourceTransaction_{service_version}.xsd", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,7 @@ def get_user_enrollments(user): | |
for program_enrollment in program_enrollments | ||
) | ||
) | ||
program_course_ids = set(course.id for course in program_courses) # noqa: C401 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the set might have been intentional here for unique values. |
||
program_course_ids = {course.id for course in program_courses} | ||
course_run_enrollments = ( | ||
CourseRunEnrollment.objects.select_related("run__course__coursepage", "company") | ||
.filter(user=user) | ||
|
@@ -316,7 +316,7 @@ def defer_enrollment( | |
to_run = CourseRun.objects.get(courseware_id=to_courseware_id) | ||
if from_enrollment.run == to_run: | ||
raise ValidationError( | ||
"Cannot defer to the same course run (run: {})".format(to_run.courseware_id) # noqa: EM103, UP032 | ||
f"Cannot defer to the same course run (run: {to_run.courseware_id})" # noqa: EM102 | ||
) | ||
if not force and not to_run.is_not_beyond_enrollment: | ||
raise ValidationError( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -282,14 +282,13 @@ def test_course_view( # noqa: PLR0913 | |
url = reverse("user-dashboard") | ||
class_name = "enrolled" | ||
|
||
# Note: UTF-8 is the default encoding in Python 3. | ||
assert ( | ||
f'<a class="enroll-button {class_name}" href="{url}">'.encode("utf-8") # noqa: UP012 | ||
in resp.content | ||
f'<a class="enroll-button {class_name}" href="{url}">'.encode() in resp.content | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing here, Does python upgrade complain about it? explicitly adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is also made by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My answer to this would be similar to #3089 (comment) |
||
) is has_button | ||
assert ( | ||
"Please Sign In to MITx PRO to enroll in a course".encode("utf-8") # noqa: UP012 | ||
in resp.content | ||
) is (is_anonymous and has_product and has_unexpired_run) | ||
assert (b"Please Sign In to MITx PRO to enroll in a course" in resp.content) is ( | ||
is_anonymous and has_product and has_unexpired_run | ||
) | ||
|
||
|
||
@pytest.mark.parametrize("is_enrolled", [True, False]) | ||
|
@@ -348,14 +347,13 @@ def test_program_view( # noqa: PLR0913 | |
url = reverse("user-dashboard") | ||
class_name = "enrolled" | ||
|
||
# Note: UTF-8 is the default encoding in Python 3. | ||
assert ( | ||
f'<a class="enroll-button {class_name}" href="{url}">'.encode("utf-8") # noqa: UP012 | ||
in resp.content | ||
f'<a class="enroll-button {class_name}" href="{url}">'.encode() in resp.content | ||
) is has_button | ||
assert ( | ||
"Please Sign In to MITx PRO to enroll in a course".encode("utf-8") # noqa: UP012 | ||
in resp.content | ||
) is (is_anonymous and has_product and has_unexpired_run) | ||
assert (b"Please Sign In to MITx PRO to enroll in a course" in resp.content) is ( | ||
is_anonymous and has_product and has_unexpired_run | ||
) | ||
|
||
|
||
def test_user_enrollments_view(mocker, client, user): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -510,7 +510,7 @@ def get_edx_api_client(user, ttl_in_seconds=OPENEDX_AUTH_DEFAULT_TTL_IN_SECONDS) | |
auth = get_valid_edx_api_auth(user, ttl_in_seconds=ttl_in_seconds) | ||
except OpenEdxApiAuth.DoesNotExist: | ||
raise NoEdxApiAuthError( # noqa: B904 | ||
"{} does not have an associated OpenEdxApiAuth".format(str(user)) # noqa: EM103, UP032 | ||
f"{user!s} does not have an associated OpenEdxApiAuth" # noqa: EM102 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
) | ||
return EdxApi( | ||
{"access_token": auth.access_token}, | ||
|
@@ -815,7 +815,7 @@ def create_oauth_application(): | |
defaults=dict( # noqa: C408 | ||
redirect_uris=urljoin( | ||
settings.OPENEDX_BASE_REDIRECT_URL, | ||
"/auth/complete/{}/".format(settings.MITXPRO_OAUTH_PROVIDER), # noqa: UP032 | ||
f"/auth/complete/{settings.MITXPRO_OAUTH_PROVIDER}/", | ||
), | ||
client_type="confidential", | ||
authorization_grant_type="authorization-code", | ||
|
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.
We can even replace it with f-string
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 reason
pyupgrade
did not convert this into an f-string is likely due to its design choice to prioritize readability. According to the pyupgrade documentation, it intentionally avoids converting expressions intof-strings
if doing so would make the expression longer or if the substitution parameters are sufficiently complicated, as this can decrease readability.In this case, the expression might have been considered sufficiently complex, or the transformation into an f-string might have made it less readable, which is why pyupgrade retained the .format() syntax.
However, if you believe an f-string would improve clarity in this case, I'm open to making the change manually. What do you think?
This holds true for similar comments you mentioned regarding
f-string
wherepyupgrade
intentionally avoids converting expressions intof-strings
.