Implement Standalone Convert; Enum Handling; User Edge/Connection Classes; Column Alias Handling - #25
Conversation
f892174 to
a9c7e35
Compare
|
up |
mattalbr
left a comment
There was a problem hiding this comment.
Thanks for the change! I'm still new on the project, so I left a few comments but added Tim and Erik as reviewers for their expertise.
If you could also add a RELEASE.md file, pushing a new commit should trigger all of the new automated testing that I got working this week (feel free to ignore the Lint step which I haven't gotten green yet).
| """ | ||
| if isinstance(column.type, Enum): | ||
| type_annotation = column.type.python_type | ||
| type_annotation = strawberry.enum(column.type.python_type) |
There was a problem hiding this comment.
From my reading of strawberry.enum it should be okay to wrap a type twice, but it would be great to have a test that does this. E.g. in my company, we already annotate all of our enums with @strawberry.enum, and I'm unsure if that would set python_type to the wrapped or unwrapped enum, and what would happen here.
Just a test that does
@strawbery.enum
class MyEnum(enum.StrEnum):
FOO = "FOO"
class MyModel(Base):
my_enum = Column(Enum(MyEnum))
There was a problem hiding this comment.
Another thought for the tests: what does that second wrapping do to any of the initial settings? e.g. If I @strawberry.enum(name="DifferentName") does it still respect the overwritten name?
| extra_sqlalchemy_type_to_strawberry_type_map: Optional[ | ||
| Mapping[Type[TypeEngine], Type[Any]] | ||
| ] = None, | ||
| edge_type: Type = None, |
There was a problem hiding this comment.
Added Erik to comment on this, but we should probably at the very least have a Protocol that these types should follow
| use_federation=False, | ||
| ) -> Any: | ||
| """ | ||
| Do type conversion. Usually accessed using typical .type decorator. But |
There was a problem hiding this comment.
I think if this is to be part of the public API, we should probably beef up the docstring a bit. I still don't really understand when someone would need this, and why someone wouldn't just call Employee = mapper.type(models.Employee)(Employee)
At the least, exercising this in a unit test to have some basic API-level test coverage and demonstrate the usage could be nice, but might be worth waiting until Erik or Tim weighs in.
d6c9ed4 to
2e1b78f
Compare
|
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature.Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) |
TimDumol
left a comment
There was a problem hiding this comment.
- Change PR title and description? Seems to only have the edge/connection type changes
- It's been a while since I've touched this code, but don't we need a distinct edge / connection type for each relationship (or at least, node type in a relationship)? e.g., if Book -> Authors and Book -> Reviews, you'd need a different edge / connection type for Author and Review, which this doesn't seem to support.
|
Hey, let's delay the enum part of this until we've standardized strawberry-graphql/strawberry#3543 |
Description
This includes a number of changes, including:
mapper.typenow callsmapper.convert- this allowsmapper.convertto be called within user code, which makes the relevant functionality accessible where a decorator is not appropriate.strawberry.enumis called against SQLAlchemy Enum columns.Types of Changes
Issues Fixed or Closed by This PR
Checklist