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

Fixed #33143 -- Raised RuntimeWarning when performing import-time queries. #16923

Merged
merged 1 commit into from Aug 21, 2023

Conversation

PoByBolek
Copy link
Contributor

Ticket #33143

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Great work! I would love to see this landed, I have made the mistake of issuing queries at import time more than once
😊

django/__init__.py Outdated Show resolved Hide resolved
django/__init__.py Outdated Show resolved Hide resolved
docs/ref/applications.txt Outdated Show resolved Hide resolved
tests/apps/tests.py Outdated Show resolved Hide resolved
django/__init__.py Outdated Show resolved Hide resolved
django/__init__.py Outdated Show resolved Hide resolved
docs/ref/applications.txt Show resolved Hide resolved
@PoByBolek PoByBolek force-pushed the main branch 6 times, most recently from 402b19f to 4da05ac Compare June 18, 2023 15:00
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@PoByBolek Thanks 👍 I left comments.

docs/releases/5.0.txt Outdated Show resolved Hide resolved
tests/apps/query_performing_app/apps.py Outdated Show resolved Hide resolved
tests/apps/query_performing_app/apps.py Outdated Show resolved Hide resolved
tests/apps/tests.py Outdated Show resolved Hide resolved
django/__init__.py Outdated Show resolved Hide resolved
tests/apps/tests.py Outdated Show resolved Hide resolved
django/__init__.py Outdated Show resolved Hide resolved
docs/ref/applications.txt Outdated Show resolved Hide resolved
@PoByBolek PoByBolek force-pushed the main branch 3 times, most recently from 273a6e3 to 26fc2f9 Compare July 12, 2023 12:42
@PoByBolek
Copy link
Contributor Author

@felixxm thanks for the review! I left a few questions to some of your comments. Could you have another look please?

@PoByBolek PoByBolek requested a review from felixxm July 12, 2023 12:45
django/__init__.py Outdated Show resolved Hide resolved
django/db/backends/utils.py Outdated Show resolved Hide resolved
@felixxm felixxm self-assigned this Aug 2, 2023
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@PoByBolek Thanks for updates 👍 Looks great, we're almost there.

django/__init__.py Outdated Show resolved Hide resolved
django/db/backends/utils.py Outdated Show resolved Hide resolved
tests/apps/query_performing_app/apps.py Outdated Show resolved Hide resolved
tests/apps/query_performing_app/apps.py Outdated Show resolved Hide resolved
tests/apps/tests.py Outdated Show resolved Hide resolved
tests/apps/tests.py Outdated Show resolved Hide resolved
@PoByBolek
Copy link
Contributor Author

PoByBolek commented Aug 3, 2023

@felixxm Is there something weird going on with Jenkins? It keeps building commit 927a54a from three weeks ago. So it doesn't execute any of the new tests. Not even those that should fail on purpose... Is this because of the conflict in docs/releases/5.0.txt?

grafik

tests/inspectdb/models.py Outdated Show resolved Hide resolved
Copy link
Sponsor Member

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

Some docs suggestions.

docs/ref/applications.txt Outdated Show resolved Hide resolved
docs/ref/applications.txt Outdated Show resolved Hide resolved
docs/ref/applications.txt Outdated Show resolved Hide resolved
docs/ref/applications.txt Outdated Show resolved Hide resolved
docs/ref/applications.txt Outdated Show resolved Hide resolved
django/db/backends/utils.py Outdated Show resolved Hide resolved
django/db/backends/utils.py Outdated Show resolved Hide resolved
@PoByBolek
Copy link
Contributor Author

Just a small process question: should I squash and rebase my commits onto the current main branch? Then I could add back the warning to the release notes (bc4d15c) without introducing conflicts and Jenkins would build the correct commit (instead of the last one that didn't have any conflicts).

@felixxm
Copy link
Member

felixxm commented Aug 8, 2023

Just a small process question: should I squash and rebase my commits onto the current main branch?

You can squash & rebase, or only rebase and I will squash when making final edits.

@felixxm felixxm changed the title Fixed #33143 -- Added a warning when performing import-time queries Fixed #33143 -- Raised RuntimeWarning when performing import-time queries. Aug 21, 2023
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@PoByBolek Thanks 👍 Welcome aboard ⛵

I pushed small edits and added tests for executemany().

@felixxm felixxm merged commit fbd1643 into django:main Aug 21, 2023
35 checks passed
@felixxm felixxm temporarily deployed to schedules August 22, 2023 02:44 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants