Skip to content

#2628: types.Variant() has a __repr__ usable by Alembic#9

Closed
ijl wants to merge 1 commit intozzzeek:masterfrom
ijl:master
Closed

#2628: types.Variant() has a __repr__ usable by Alembic#9
ijl wants to merge 1 commit intozzzeek:masterfrom
ijl:master

Conversation

@ijl
Copy link
Copy Markdown
Contributor

@ijl ijl commented Jun 21, 2013

This pull request is meant to fix Trac ticket #2628 'need a working repr for Variant".

Note that generic_repr() now builds kwargs into the repr by looking for a kwargs_signature attribute in the to_inspect object. This complements inspect.getargspec() in building the list of all arguments and whether they are non-default. So any type that accepts kwargs will need to define this attribute in order to have a comprehensive repr. That's something to keep up to date in the future, but the only alternative I saw was to refactor any type using an _arg or *_kwargs into named arguments, which meant backwards-incompatibility for those Enums at the least.

Please see the tests in test_repr().

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jun 21, 2013

This isn't correct. A migration script needs to be just as database agnostic as anything else using SQL constructs, so it is wrong for Variant to repr() itself as some totally different type, losing all the arguments we've used to construct it. Variant should have a traditional repr() function and in fact that is probably not sufficient for Alembic's purposes, and Alembic itself should have some special logic for the Variant type.

Ideally, in a migration script, we'd see this:

op.add_column(Column("mycol", Variant(Enum, {"mysql": mysql.ENUM(...), "postgresql": postgresql.ENUM(...)})

Which kind of means that Alembic itself should really have a hand in doing the reproduction of a Variant, because you'll note it needs to have those dialect namespaces like mysql. and postgresql. in there. I'd propose that my ticket itself is incorrect, because this isn't an issue that can be completely solved in SQLAlchemy.

If the __repr__() for all the Enum types need more specificity, then they should just get their own __repr__() method. There also can't be any backwards-incompatible changes like the "active_dialect" parameter.

@zzzeek zzzeek closed this Jun 21, 2013
@ijl
Copy link
Copy Markdown
Contributor Author

ijl commented Jun 21, 2013

I see. I misunderstood. Thanks.

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jun 21, 2013

s132048 pushed a commit to s132048/docs-korean-sqlalchemy that referenced this pull request Jun 5, 2019
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.

2 participants