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

chore: python requirements update #4282

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

edx-requirements-bot
Copy link
Contributor

Python requirements update.Please review the changelogs for the upgraded packages.

Deleted obsolete pull_requests:
#4277

@edx-requirements-bot edx-requirements-bot requested a review from a team March 4, 2024 00:22
@@ -462,7 +462,10 @@ def test_ingest_flow_for_preexisting_published_course_with_new_run_creation(self
CourseRunFactory(
course=course,
start=datetime.datetime(2014, 3, 1, tzinfo=UTC),
end=datetime.datetime(2024, 3, 1, tzinfo=UTC),
# 2025 end date is to ensure the course run is present among active runs and thus
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we mean 2050 here?

end=datetime.datetime(2024, 3, 1, tzinfo=UTC),
# 2025 end date is to ensure the course run is present among active runs and thus
# non-draft entries are created. If the discovery is there till 2050, you would need to update the
# tests after Jan 1, 2025.
Copy link
Contributor

Choose a reason for hiding this comment

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

2050?

Copy link
Contributor

@DawoudSheraz DawoudSheraz Mar 4, 2024

Choose a reason for hiding this comment

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

Oh, yes, 2050. wow.

@@ -46,7 +46,7 @@ def get_query_results_from_snowflake(self):
cs.execute(SNOWFLAKE_POPULATE_COURSE_LENGTH_QUERY)
rows = cs.fetchall()
for row in rows:
yield row
yield from row
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Quality change -- essentially both are equivalent but pylint is being restrictive to this format.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are not equivalent. yield from treats row as an iterable and yields individual values from it whereas yield yields the row. As an example you can set rows = [(1,2,3), (4,5,6)] and test this.

Just looking at the snippet earlier we were yielding rows of results. With this change, I expect we'd be yielding the column values in the rows. I am confused though as to why does pylint complain about this, and why do some tests not fail here. Or maybe the tests do not check for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, thanks for confirming. I misunderstood the docs.
The tests are essentially mocking the return value of get_query_results_from_snowflake, so that's why it is not caught by the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

The data from the method is essentially a list of lists. The correct option here is yield from rows instead of yield from row

def fn3():
    rows = [[1, 2, 3], [4, 5, 6]]
    yield from rows


for item in fn3():
    print(item)

@@ -47,7 +47,7 @@ def get_query_results_from_snowflake(self):
cs.execute(SNOWFLAKE_REFRESH_COURSE_REVIEWS_QUERY)
rows = cs.fetchall()
for row in rows:
yield row
yield from row
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

@DawoudSheraz DawoudSheraz force-pushed the jenkins/upgrade-python-requirements-9c8fd5a branch from ae6cbfd to 1596e7e Compare March 4, 2024 07:18
@DawoudSheraz DawoudSheraz merged commit b8e3d49 into master Mar 4, 2024
12 checks passed
@DawoudSheraz DawoudSheraz deleted the jenkins/upgrade-python-requirements-9c8fd5a branch March 4, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants