Fix postgres detect serial in autogenerate (#1479) #1486
Conversation
There was a problem hiding this comment.
Thanks, left a suggestion regarding the tests
Can you also add a changelog, similar to these one? https://github.com/sqlalchemy/alembic/tree/main/docs/build/unreleased
tests/test_postgresql.py
Outdated
| @@ -860,7 +860,7 @@ def teardown_class(cls): | |||
|
|
|||
| @provide_metadata | |||
| def _expect_default(self, c_expected, col, seq=None): | |||
| Table("t", self.metadata, col) | |||
| Table("t", self.metadata, col, schema="test_schema") | |||
There was a problem hiding this comment.
it would be better to have both cases tested.
I think you could be better to have a parameter passed to the outside to enable /disable the schema. Then we could have separate tests that run or not the schema (if could also use combinations())
|
change log and more test has been added |
|
thanks. I'll bring in it gerrit were the complete pipeline can run |
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 24df8f9 of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change 24df8f9: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5333 |
There was a problem hiding this comment.
Michael Bayer (zzzeek) wrote:
This seems OK but I need to know very clearly how on earth that comparison works, and if we could perhaps have something that looks a little more explicit /safe that would be an improvement. this needs a comment at the very least how we can know this bizarre comparison is safe
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5333
|
Michael Bayer (zzzeek) wrote: it's documented here: https://www.postgresql.org/docs/current/datatype-oid.html View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5333 |
|
Michael Bayer (zzzeek) wrote: code review left on gerrit View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5333 |
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5333 has been merged. Congratulations! :) |
Fixes: #1479
Description
In #73, it tries to detact postgresql serial in autogenerate, so it won't take
nextval('seq'::regclass)as server default for that column.But it takes not effect for tables not in search path. This PR fixed it.
Checklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>in the commit messageHave a nice day!