handle SQLAlchemy types with unimplemented python_type as typing.Any & use SQLModel.metadata as metadataref#425
handle SQLAlchemy types with unimplemented python_type as typing.Any & use SQLModel.metadata as metadataref#425danplischke wants to merge 4 commits intoagronholm:masterfrom
Conversation
|
Wouldn't it be a lot better to just send a PR to upstream to implement |
|
Can you elaborate on adding SQLModel.metadata? |
Sure! For tables that don't have a primary key, we fall back to using SQLAlchemy Models, however due to the missing metadataref we get this: test_table = Table(
'test_table', ,
Column('test', Integer),
schema='test'
)which is missing the metadata parameter which is mandatory in SQLAlchemy. |
Sure, happy to open a PR there. |
Yes, we are planning to catch the |
@sheinbergon I think we might prefer this approach instead of just not generating any code for such tables. |
@agronholm Just what I had in mind |
fix: SQLModel.metadata as metadataref
danplischke
changed the title
So, like this? (changed the code in this PR) |
I think so, I will give a more thorough look later on today. @danplischke your contribution is much appreciated |
src/sqlacodegen/generators.py
Outdated
| try: | ||
| python_type = column_type.python_type | ||
| except NotImplementedError: | ||
| _logger.warning( |
There was a problem hiding this comment.
We don't need this warning, we'll be encountering this left and right for any non-standard use case
| python_type_name = ( | ||
| python_type.__name__ | ||
| if hasattr(python_type, "__name__") | ||
| else python_type._name |
There was a problem hiding this comment.
What's the deal with this fallback? we can we expect for python_type._name to succeed
| @@ -1435,7 +1449,7 @@ def generate_base(self) -> None: | |||
| self.base = Base( | |||
There was a problem hiding this comment.
Can you add a test to cover this behavior for SQLModels?
While this PR provides a nice behavior for SQLModels, we need to think about other generators |
| python_type_name = ( | ||
| python_type.__name__ | ||
| if hasattr(python_type, "__name__") | ||
| else python_type._name |
There was a problem hiding this comment.
Is this just to accommodate Any?
There was a problem hiding this comment.
yes, unfortunately and only in 3.9 as Any is a _SpecialForm type that is missing name
|
@danplischke this contribution is important to us. While you can try and tackle both issues covered in the same PR, though I suggest we split it into 2 parts:
If you require assistance, or want to split the work, let me know |
|
@danplischke hi. I've ported (and modified) your work to fix the If you still want to work on SQLModel fixes, please rework your PR to reflect that. |
4 participants
Changes
Fixes #423, #392
Added custom handling for Postgres TSVECTOR type, as python_type is not implemented in sqlalchemy type.
Also added metadataref for SQLModel generation, which would lead to syntax errors for fallback SQLAlchemy Tables.
Checklist
If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):
tests/) which would fail without your patchCHANGES.rst).If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.
Updating the changelog
If there are no entries after the last release, use
**UNRELEASED**as the version.If, say, your patch fixes issue #123, the entry should look like this:
If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.