Skip to content
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

Merged
merged 8 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ jobs:
- name: Set up Python
uses: actions/setup-python@v4
with:
python-version: "3.9.15"
python-version: "3.12.4"
cache: "poetry"

- name: Install dependencies
Expand Down
6 changes: 3 additions & 3 deletions .secrets.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -266,16 +266,16 @@
"filename": "users/api.py",
"hashed_secret": "f205fad2d580e981bb53020ed8f77c0cb7c35014",
"is_verified": false,
"line_number": 165
"line_number": 163
},
{
"type": "Base64 High Entropy String",
"filename": "users/api.py",
"hashed_secret": "915109282e07e7e73fb6939dd221f675e60d118f",
"is_verified": false,
"line_number": 168
"line_number": 166
}
]
},
"generated_at": "2024-05-22T09:32:37Z"
"generated_at": "2024-08-06T16:53:29Z"
}
2 changes: 1 addition & 1 deletion Dockerfile
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
Expand Down
2 changes: 1 addition & 1 deletion affiliate/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
class AffiliateFactory(DjangoModelFactory):
"""Factory for Affiliate"""

code = factory.Sequence("affiliate-code-{0}".format)
code = factory.Sequence("affiliate-code-{}".format)
name = fuzzy.FuzzyText(prefix="Affiliate ", length=30)

class Meta:
Expand Down
2 changes: 1 addition & 1 deletion authentication/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def process_exception(self, request, exception):
url = self.get_redirect_uri(request, exception)

if url: # noqa: RET503
url += ("?" in url and "&" or "?") + "message={0}&backend={1}".format( # noqa: UP030, UP032
url += ("?" in url and "&" or "?") + "message={}&backend={}".format( # noqa: UP032
Copy link
Contributor

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

Copy link
Contributor Author

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 into f-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 where pyupgrade intentionally avoids converting expressions into f-strings.

quote(message), backend_name
)
return redirect(url)
2 changes: 1 addition & 1 deletion authentication/pipeline/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ def create_user_via_email(
created_user = create_user_with_generated_username(serializer, username)
if created_user is None:
raise IntegrityError( # noqa: TRY301
"Failed to create User with generated username ({})".format(username) # noqa: EM103, UP032
f"Failed to create User with generated username ({username})" # noqa: EM102
)
except Exception as exc:
raise UserCreationFailedException(backend, current_partial) from exc
Expand Down
8 changes: 4 additions & 4 deletions cms/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class Meta:
class ProgramPageFactory(wagtail_factories.PageFactory):
"""ProgramPage factory class"""

title = factory.Sequence("Test page - Program {0}".format)
title = factory.Sequence("Test page - Program {}".format)
program = factory.SubFactory(ProgramFactory, page=None)
subhead = factory.fuzzy.FuzzyText(prefix="Subhead ")
thumbnail_image = factory.SubFactory(wagtail_factories.ImageFactory)
Expand Down Expand Up @@ -99,7 +99,7 @@ def post_gen(obj, create, extracted, **kwargs): # noqa: ARG002, N805
class CoursePageFactory(wagtail_factories.PageFactory):
"""CoursePage factory class"""

title = factory.Sequence("Test page - Course {0}".format)
title = factory.Sequence("Test page - Course {}".format)
course = factory.SubFactory(CourseFactory, page=None)
subhead = factory.fuzzy.FuzzyText(prefix="Subhead ")
thumbnail_image = factory.SubFactory(wagtail_factories.ImageFactory)
Expand Down Expand Up @@ -131,7 +131,7 @@ class ExternalCoursePageFactory(wagtail_factories.PageFactory):

course = factory.SubFactory(CourseFactory, page=None)

title = factory.Sequence("Test page - External Course {0}".format)
title = factory.Sequence("Test page - External Course {}".format)
external_marketing_url = factory.Faker("uri")
marketing_hubspot_form_id = factory.Faker("bothify", text="??????????")
subhead = factory.fuzzy.FuzzyText(prefix="Subhead ")
Expand Down Expand Up @@ -160,7 +160,7 @@ class ExternalProgramPageFactory(wagtail_factories.PageFactory):

program = factory.SubFactory(ProgramFactory, page=None)

title = factory.Sequence("Test page - External Program {0}".format)
title = factory.Sequence("Test page - External Program {}".format)
external_marketing_url = factory.Faker("uri")
marketing_hubspot_form_id = factory.Faker("bothify", text="??????????")
subhead = factory.fuzzy.FuzzyText(prefix="Subhead ")
Expand Down
6 changes: 3 additions & 3 deletions cms/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ def get_context(self, request, *args, **kwargs):
.exclude(Q(category=UPCOMING_WEBINAR) & Q(date__lt=now_in_utc().date()))
.order_by("-category", "date")
)
webinars_dict = defaultdict(lambda: []) # noqa: PIE807
webinars_dict = defaultdict(list)
for webinar in webinars:
webinar.detail_page_url = webinar.detail_page_url(request)
webinars_dict[webinar.category].append(webinar)
Expand Down Expand Up @@ -1028,7 +1028,7 @@ def get_url_parts(self, request=None):
# of the Course/Program instead (e.g.: "/courses/course-v1:edX+DemoX+Demo_Course")
re.sub(
self.slugged_page_path_pattern,
r"\1{}\3".format(self.product.readable_id), # noqa: UP032
rf"\1{self.product.readable_id}\3",
url_parts[2],
),
)
Expand Down Expand Up @@ -1508,7 +1508,7 @@ class Meta:
def can_create_at(cls, parent):
# You can only create one of these page under course / program.
return (
super(CourseProgramChildPage, cls).can_create_at(parent) # noqa: UP008
super().can_create_at(parent)
and parent.get_children().type(cls).count() == 0
)

Expand Down
5 changes: 3 additions & 2 deletions compliance/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file open is readable by default but explicitly adding "r" adds more readability for all code readers. Did Python upgrade complain about it? Same goes for the line below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not complain but the open() function in Python defaults to read mode ("r"), which pyupgrade makes explicitly specifying "r" to redundant-open-mode/unnecessary and removes it. So we can go any of the following options:

  1. Keep the Explicit "r"
    • We can keep the explicit "r" for readability if that's preferred. Although pyupgrade removed it because it's not necessary, adding it back can make the intent clearer to all code readers, especially those who may not be aware of the default behavior.
  2. Adopt Python's Default Behavior

Note: The first option will be applied consistently to all instances where this change is present and/or explicitly mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

It does not complain but the open() function in Python defaults to read mode ("r"), which pyupgrade makes explicitly specifying "r" to redundant-open-mode/unnecessary and removes it. So we can go any of the following options:

I think if pyupgrade complains about it that should mean that the python upgrade does complain. In, which case maybe we can remove the r and add a code comment above this line saying that the file open is read-only by default.

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",
Expand Down
4 changes: 2 additions & 2 deletions courses/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Expand Down Expand Up @@ -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(
Expand Down
3 changes: 1 addition & 2 deletions courses/credentials.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
"""Digital courseware credentials"""

import logging
from typing import Union
from urllib.parse import urljoin

from django.conf import settings
Expand Down Expand Up @@ -81,7 +80,7 @@ def build_course_run_credential(certificate: CourseRunCertificate) -> dict:


def build_digital_credential(
certificate: Union[ProgramCertificate, CourseRunCertificate], # noqa: FA100
certificate: ProgramCertificate | CourseRunCertificate, # noqa: FA102
learner_did: LearnerDID,
) -> dict:
"""Function for building certificate digital credentials"""
Expand Down
6 changes: 3 additions & 3 deletions courses/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class ProgramRunFactory(DjangoModelFactory):
"""Factory for ProgramRuns"""

program = factory.SubFactory(ProgramFactory)
run_tag = factory.Sequence("R{0}".format)
run_tag = factory.Sequence("R{}".format)

class Meta:
model = ProgramRun
Expand All @@ -77,7 +77,7 @@ class CourseFactory(DjangoModelFactory):
program = factory.SubFactory(ProgramFactory)
position_in_program = None # will get populated in save()
title = fuzzy.FuzzyText(prefix="Course ")
readable_id = factory.Sequence("course-{0}".format)
readable_id = factory.Sequence("course-{}".format)
platform = factory.SubFactory(PlatformFactory)
live = True

Expand All @@ -96,7 +96,7 @@ class CourseRunFactory(DjangoModelFactory):
course = factory.SubFactory(CourseFactory)
title = factory.LazyAttribute(lambda x: "CourseRun " + FAKE.sentence()) # noqa: ARG005
courseware_id = factory.Sequence(lambda number: f"course:v{number}+{FAKE.slug()}")
run_tag = factory.Sequence("R{0}".format)
run_tag = factory.Sequence("R{}".format)
courseware_url_path = factory.Faker("uri")
start_date = factory.Faker(
"date_time_this_month", before_now=True, after_now=False, tzinfo=timezone.utc
Expand Down
4 changes: 2 additions & 2 deletions courses/management/commands/defer_enrollment.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,12 @@ def handle(self, *args, **options): # noqa: ARG002
if isinstance(exc, CourseRunEnrollment.DoesNotExist):
message = f"'from' course run enrollment does not exist ({from_courseware_id})"
elif isinstance(exc, CourseRun.DoesNotExist):
message = "'to' course does not exist ({})".format(to_courseware_id) # noqa: UP032
message = f"'to' course does not exist ({to_courseware_id})"
else:
message = str(exc)
raise CommandError(message) # noqa: B904
except ValidationError as exc:
raise CommandError("Invalid enrollment deferral - {}".format(exc)) # noqa: B904, EM103, UP032
raise CommandError(f"Invalid enrollment deferral - {exc}") # noqa: B904, EM102
else:
if not to_enrollment:
raise CommandError(
Expand Down
2 changes: 1 addition & 1 deletion courses/management/commands/revoke_certificate.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def handle(self, *args, **options): # noqa: ARG002

if updated:
msg = "Certificate for {} has been {}".format(
"run: {}".format(run) if run else "program: {}".format(program), # noqa: UP032
f"run: {run}" if run else f"program: {program}",
"revoked" if revoke else "un-revoked",
)
self.stdout.write(self.style.SUCCESS(msg))
Expand Down
6 changes: 2 additions & 4 deletions courses/management/commands/sync_grades_and_certificates.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,9 @@ def handle( # noqa: C901, PLR0915
else:
grade_status = "already exists"

grade_summary = ["passed: {}".format(course_run_grade.passed)] # noqa: UP032
grade_summary = [f"passed: {course_run_grade.passed}"]
if override_grade is not None:
grade_summary.append(
"value override: {}".format(course_run_grade.grade) # noqa: UP032
)
grade_summary.append(f"value override: {course_run_grade.grade}")

if created_cert:
cert_status = "created"
Expand Down
2 changes: 1 addition & 1 deletion courses/management/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ def fetch_enrollment(user, command_options):
enrollment = CourseRunEnrollment.all_objects.filter(**query_params).first()

if not enrollment:
raise CommandError("Enrollment not found for: {}".format(enrolled_obj)) # noqa: EM103, UP032
raise CommandError(f"Enrollment not found for: {enrolled_obj}") # noqa: EM102
if not enrollment.active and not force:
raise CommandError(
"The given enrollment is not active ({}).\n" # noqa: EM103, UP032, RUF100
Expand Down
2 changes: 1 addition & 1 deletion courses/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ def catalog_image_url(self):


validate_url_path_field = RegexValidator(
r"^[{}]+$".format(detail_path_char_pattern), # noqa: UP032
rf"^[{detail_path_char_pattern}]+$",
f"This field is used to produce URL paths. It must contain only characters that match this pattern: [{detail_path_char_pattern}]",
)

Expand Down
4 changes: 2 additions & 2 deletions courses/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -371,12 +371,12 @@ def get_instructors(self, instance):

def get_topics(self, instance):
"""List all topics in all courses in the program"""
topics = set( # noqa: C401
topics = {
topic.name
for course in instance.courses.all()
if course.page
for topic in course.page.topics.all()
)
}
return [{"name": topic} for topic in sorted(topics)]

def get_time_commitment(self, instance):
Expand Down
2 changes: 1 addition & 1 deletion courses/serializers_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def test_serialize_program( # noqa: PLR0913

non_live_run = CourseRunFactory.create(
course=course1,
end_date=datetime.max.astimezone(timezone.utc),
end_date=datetime.max.replace(tzinfo=timezone.utc),
expiration_date=None,
live=False,
)
Expand Down
22 changes: 10 additions & 12 deletions courses/views_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, Does python upgrade complain about it? explicitly adding utf-8 makes it more readable. Same for the places below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is also made by pyupgrade to encode string to bytes literal as UTF-8 is the default encoding in Python 3. This resolves unnecessary-encode-utf8 (UP012))

Copy link
Contributor

Choose a reason for hiding this comment

The 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])
Expand Down Expand Up @@ -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):
Expand Down
4 changes: 2 additions & 2 deletions courseware/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think __str__ of the user model would automatically be called in an f-string.

)
return EdxApi(
{"access_token": auth.access_token},
Expand Down Expand Up @@ -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",
Expand Down
17 changes: 8 additions & 9 deletions ecommerce/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
import uuid
from base64 import b64encode
from collections import defaultdict
from collections.abc import Iterable
from datetime import timedelta
from typing import Iterable, NamedTuple, Optional # noqa: UP035
from typing import NamedTuple
from urllib.parse import quote_plus, urljoin

from django.conf import settings
Expand Down Expand Up @@ -836,11 +837,9 @@ def enroll_user_in_order_items(order):
):
voucher_target = voucher.product.content_object
voucher_enrollment = first_or_none(
( # noqa: UP034
enrollment
for enrollment in successful_run_enrollments
if enrollment.run == voucher_target
)
enrollment
for enrollment in successful_run_enrollments
if enrollment.run == voucher_target
)
if voucher_enrollment is not None:
voucher.enrollment = voucher_enrollment
Expand Down Expand Up @@ -999,9 +998,9 @@ class ValidatedBasket(NamedTuple):
basket: Basket
basket_item: BasketItem
product_version: ProductVersion
coupon_version: Optional[CouponVersion] # noqa: FA100
run_selection_ids: Optional[Iterable[int]] # noqa: FA100
data_consent_users: Optional[Iterable[DataConsentUser]] # noqa: FA100
coupon_version: CouponVersion | None # noqa: FA102
run_selection_ids: Iterable[int] | None # noqa: FA102
data_consent_users: Iterable[DataConsentUser] | None # noqa: FA102


def _validate_basket_contents(basket):
Expand Down
Loading
Loading