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

Add Django 4.1-4.2 and Python 3.11 support #229

Merged
merged 8 commits into from
Jul 11, 2023

Conversation

Andrew-Chen-Wang
Copy link
Collaborator

@Andrew-Chen-Wang Andrew-Chen-Wang commented Apr 4, 2023

  • Increment version to 4.6.0

Add Django 4.2 and Python 3.11 support

Drops Django 2.2 and 4.0

Fixes #228

@Andrew-Chen-Wang Andrew-Chen-Wang changed the title Add Django 4.1-4.2 and Python 3.11 support (Fixes #228) Add Django 4.1-4.2 and Python 3.11 support Apr 4, 2023
@coveralls
Copy link

coveralls commented Apr 4, 2023

Pull Request Test Coverage Report for Build 5515266644

  • 18 of 18 (100.0%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.8%) to 98.151%

Files with Coverage Reduction New Missed Lines %
cachalot/utils.py 1 95.76%
Totals Coverage Status
Change from base Build 4403102470: 0.8%
Covered Lines: 690
Relevant Lines: 703

💛 - Coveralls

@Andrew-Chen-Wang
Copy link
Collaborator Author

Going to need to detect when we go into a transaction since Django added transaction BEGIN and COMMIT to the captured queries

@oriolclosa
Copy link

Came here to make a PR for it, found someone had done it already, amazing 😄🎉.

@Andrew-Chen-Wang
Copy link
Collaborator Author

@oriolclosa It is incomplete. Has a ton of errors due to the addition of BEGIN and COMMIT in sql query history.

Would appreciate it if you could help make a PR!

@creyD
Copy link
Contributor

creyD commented Apr 25, 2023

@Andrew-Chen-Wang When do you think this will be done?

@Andrew-Chen-Wang
Copy link
Collaborator Author

hi i haven't found the time to fix it up yet. For the most part it works on Django 4.2 but the test cases fail since it's validating output that Django users don't need to care about

fwiw no code base changes are needed but haven't had time to fix tests

@creyD
Copy link
Contributor

creyD commented Apr 26, 2023

@Andrew-Chen-Wang Thanks for the quick response!

@creyD
Copy link
Contributor

creyD commented May 8, 2023

@Andrew-Chen-Wang When do you think this could be released?

@Andrew-Chen-Wang
Copy link
Collaborator Author

hey I think I just don't have the time to look over OSS for now @BertrandBordage I think another maintainer is needed

if anyone else wants to make a branch to complete this pr, i'm happy to review it. Thanks:)

@jacklinke
Copy link
Contributor

Submitted PR #234 for this branch - it should resolve most if not all of the test failures.

@jacklinke
Copy link
Contributor

Well, that's embarrassing 😵‍💫

Overlooked a very simple error else else in tests/write.py. Fixed in #235

@Hopiu
Copy link
Contributor

Hopiu commented Jun 2, 2023

Are there any remaining components required to fully support 4.2? @jacklinke, are you actively working on this? If not, I am happy to assist in resolving any issues with the failing tests.

@Andrew-Chen-Wang
Copy link
Collaborator Author

Andrew-Chen-Wang commented Jun 2, 2023

It seems like the only things missing is correcting the number of queries expected when catching the amount. The issue is that Django 4.2 catches transactions and logs them (BEGIN, COMMIT, etc.). I think the most simplest solution is to just update the query count, but this can change anytime.

Instead we need some regex to catch whenever transaction related terminology are being recorded and ignore it/exclude it from the counter since cachalot doesn't really care about that.

@denizs
Copy link

denizs commented Jun 29, 2023

I was lurking around, but I figured I might as well ask 😅 : Is there anyone actively working on getting this into a reviewable state ATM? I am not too familiar with the internals of this project, but with a couple of pointers, I might be able to bring this one home.

@Andrew-Chen-Wang
Copy link
Collaborator Author

hi @denizs! Thanks for offering to help.

If you'd like to help, create a PR pointing to the branch. The only tests are failing are PostgreSQL related. You can take a look at the tests themselves as I suspect they are the culprit.

@Hopiu
Copy link
Contributor

Hopiu commented Jun 29, 2023

I completely forgot about those missing tests. I'll take a look at them, as soon as I have some time available.
@denizs if you are in a hurry, you are welcome to take over.

@Hopiu Hopiu mentioned this pull request Jul 7, 2023
* Added a FilteredTransactionTestCase, updated tests.

* Added more filtered tests

* Removed Python <3.10 from the tox envlist for Django main.

* Enhanced error message to include current database vendor of the connection for better easier troubleshooting.

* Use python3.10 for GH workflows, fix postgres range issues in tests.

* Check if psycopg3 is in use, if so, add psycopg3 types to CACHABLE_PARAM_TYPES.

---------

Co-authored-by: Benedikt Willi <[email protected]>
@Andrew-Chen-Wang
Copy link
Collaborator Author

thank you @jacklinke @Hopiu for your contributions!

@Andrew-Chen-Wang Andrew-Chen-Wang merged commit 2697986 into master Jul 11, 2023
@Andrew-Chen-Wang Andrew-Chen-Wang deleted the andrew-wang/dj4.2 branch July 11, 2023 03:33
@creyD
Copy link
Contributor

creyD commented Jul 12, 2023

Thanks everyone for your work! When can we expect a new release featuring these changes?

@denisSurkov
Copy link
Contributor

@Andrew-Chen-Wang , sorry to bother you, but I think you might help with realeasing new version?

@Andrew-Chen-Wang
Copy link
Collaborator Author

Thank you for all the reminders, and apologies for the delay. 2.6.0 is now published. Thank you again for all the contributions everyone to getting this through!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Django 4.2
8 participants