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

mysql asyncmy driver #382

Merged
merged 10 commits into from
Nov 15, 2021
Merged

mysql asyncmy driver #382

merged 10 commits into from
Nov 15, 2021

Conversation

aminalaee
Copy link
Member

@aminalaee aminalaee commented Sep 6, 2021

Fixes #244 .
Adding asyncmy as another mysql driver which is more up to date and faster in some areas.

  • Adding asyncmy
  • Fixing all tests (raw queries are failing)
  • Update the docs

@aminalaee aminalaee requested a review from a team September 6, 2021 12:16
@aminalaee aminalaee marked this pull request as draft September 6, 2021 12:16
@aminalaee aminalaee marked this pull request as ready for review September 11, 2021 07:16
@aminalaee aminalaee force-pushed the mysql-asyncmy branch 2 times, most recently from 543df08 to 2e06268 Compare September 12, 2021 07:15
@dkharyna
Copy link

dkharyna commented Oct 8, 2021

@aminalaee @PrettyWood when the changes will be released?

@aminalaee
Copy link
Member Author

@dkharyna I'm waiting for feedback.

@dkharyna
Copy link

dkharyna commented Oct 8, 2021

@dkharyna I'm waiting for feedback.

I see. Thanks for the quick respond.

@aminalaee aminalaee removed the request for review from a team October 19, 2021 14:52
@aminalaee
Copy link
Member Author

@dkharyna In the meantime, you can also go through this and provide any feedback if you have.

@kirilldd2
Copy link

@aminalaee Hi! The main maintainer seems to be AWOL. Is there any way to keep the repo going? Being without python 3.10 support is quite unpleasant :)

@aminalaee aminalaee requested review from Kludex and a team October 25, 2021 07:23
@aminalaee
Copy link
Member Author

@Ioloman Hi
I'm trying to spend more time on this repo, hopefully it will get more features/fixes.

As for Python 3.10 it's being added to all repos. I'll get that ready.

@aminalaee aminalaee marked this pull request as draft October 25, 2021 17:58
@Kludex
Copy link
Sponsor Member

Kludex commented Oct 25, 2021

You probably want to split some modifications on a separated PR, like postgres image upgrade, Python 3.10 CI support, etc

I'm not much into this project, so you can also ignore my words 😗

@aminalaee
Copy link
Member Author

aminalaee commented Oct 25, 2021

@Kludex You're right somehow. I can't seem to get everything working now :D
Actually it was working before but simplified the actions, and now it's failing again.

It's a bit complicated, I can add python 3.10 support as I'm limiting 3.6 for another one. But you're right some of it could be done separately.

@aminalaee aminalaee marked this pull request as ready for review October 25, 2021 20:29
Copy link

@gordthompson gordthompson left a comment

Choose a reason for hiding this comment

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

This seems to work fine for me in my little FastAPI test rig at

https://github.com/gordthompson/fastapi-crud-demo

I would prefer that mysql://… defaulted to asyncmy instead of aiomysql, but I understand that you might not want to do that right away for fear of breaking stuff. Maybe eventually? (It really does look like the whole "aio*" family of drivers has been orphaned.)

@aminalaee
Copy link
Member Author

@gordthompson Thank you for the review.

TBH, I think it would be better to just remove the default database drivers. They can be confusing, we can stick to explicit drivers.

@aminalaee
Copy link
Member Author

@tomchristie Friendly reminder :)

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

I'm just going to leave these ones up to your call @aminalaee.

Generally worth trying to keep different aspects into different PRs (Eg. I notice there's some changes to the development scripts here, which really could be kept separate to the asyncmy driver addition.) But I'm not bothered enough about that to block this from being able to go in, if you think it's ready.

@aminalaee
Copy link
Member Author

I'm just going to leave these ones up to your call @aminalaee.

Generally worth trying to keep different aspects into different PRs (Eg. I notice there's some changes to the development scripts here, which really could be kept separate to the asyncmy driver addition.) But I'm not bothered enough about that to block this from being able to go in, if you think it's ready.

Thanks for the review. You're right I could leave a little bit of that out of this MR, but as you said it wasn't that much.
Basically I had to edit the actions to allow python coverage for python 3.10 and thought now that I'm here, I'd rather fix this.

Will keep that in mind.

@tomchristie
Copy link
Member

Yup. To be fair - I do sometimes do the same myself. 🤭

@aminalaee aminalaee merged commit 89c9f3f into master Nov 15, 2021
@aminalaee aminalaee deleted the mysql-asyncmy branch November 15, 2021 14:25
@aminalaee aminalaee mentioned this pull request Jan 6, 2022
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.

Migrate away from aiomysql
6 participants