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
build(deps): switch to sqlalchemy 1.4 #299
Conversation
89ab2b9
to
8aa58f3
Compare
8aa58f3
to
36df25a
Compare
return self._row.values() | ||
@property | ||
def _mapping(self) -> asyncpg.Record: | ||
return self._row |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like your insight on this matter.
For postgres
, row.values()
was still working thanks to this Record
class but for all other backends we now need to do row._mapping.values()
with sqlalchemy >= 1.4
Do we want to:
- keep
values()
only forpostgres
- keep
values()
for all backends and we can attach the deprecatedproperty
to thesqlalchemy.Row
class - let it like this and mimic sqlalchemy API also for
postgres
Thanks for this very nice lib!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo It'd be better to keep row.values()
for all backends since looking at test changes I can see that row.keys()
is still present so row._mapping.values()
would bring an inconsistency.
Although it's sorta weird they kept row.keys()
but got rid of row.values()
in favor of row._mapping.values()
. I think you could try row._asdict()
as suggested here in SQLAlchemy issue related to 1.4.
SQLAlchemy documentation says row.keys()
is deprecated in favor of row._mapping()
so I suppose you have to add row.mapping()
for all backends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I don't really want to add row.values()
for all backends since it would need some monkeypatching or wrapper around the SQLAlchemy objects.
I reckon the idea of databases
is to keep things simple and to stay close to SQLAlchemy when possible.
This is why I went with updating postgres backend only to mimic SQLAlchemy behaviour.
But honestly I don't have a strong opinion on this so if people really want values()
, keys()
back, we can recreate the wrapper RowProxy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I started my comment with keep since that inconsistency was triggering me, although after reading SQLAlchemy's changelogs and docs I immediately changed my mind about keeping values
but did not change the original paragraph.
I do agree with decision to keep the API as close to SQLAlchemy's as possible.
Just that test having both row.values
and row._mapping.keys
misled me.
|
New release 0.4.3 to properly pin SQLAlchemy until this PR is resolved. https://pypi.org/project/databases/ (This PR should update the pin to |
@tomchristie do we want to support both 1.3 and 1.4 or 1.4 only? |
Ideally both, at least for a time. (But if that was really awkward I guess we could consider just 1.4, with some clear notes for users on which older version of |
@tomchristie Ok no problem. Is it ok if I update CI pipeline to run all tests with both versions? |
I guess dropping support for 1.3 is okay since An ugly solution to support both 1.3 and 1.4 can be seen here in SQLAlchemy-Utils 0.37. |
@Euromance Fair point. I'm okay with requiring 1.4+ then. |
Cool! I'll get back on it next Thursday when I have more time |
36df25a
to
9d6e0c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you could also test that deprecated methods throw their warnings
@Euromance Yes it was planned thanks 👍 I first wanted to have your feedback on the interface. Glad we agree! Coverage should be good now |
Do you guys have any update on the ETA on when this might get merged and released to the public? Thank you! |
Hi, is there any plan to merge this PR? When are you planning to do a release with sqlalchemy >= 1.4 support? 🙏🏼 |
@PrettyWood If you'd like to get this in then let me know and can add you to the maintainers team, and walk you through the release process. |
With pleasure @tomchristie! |
@PrettyWood - Okay, I've sent you an invite. I'll leave getting this reviewed/merged/conflict resolved/whatever up to you - tho let us know if you need anything. Then once that's done let's walk through how you can roll a release. |
40c41c2
to
bce05db
Compare
@tomchristie I rebased on |
Is this ready to be merged? Thank you! |
I am also waiting for this release |
@PrettyWood Do you want to make any other changes or are you waiting for more reviewers? |
Hi @aminalaee |
Hi, @PrettyWood |
Sqlalchemy 1.4 is out with some breaking changes (e.g.
RowProxy
got removed in favor ofRow
or the removal ofrow.values()
in favor ofrow._mapping.values()
)closes #298 and closes #348