-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
allow for directives to be added to the types #204
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request enhances the SQLAlchemy type mapper to allow for the addition of directives, improving its compatibility with GraphQL federation. The changes primarily affect the Updated class diagram for SQLAlchemy type mapperclassDiagram
class Mapper {
+type(model: Type[BaseModelType], make_interface: bool, use_federation: bool, directives: Union[Sequence[object], None] = ())
+convert(type_: Any) Any
}
note for Mapper "The 'type' method now accepts 'directives' to enhance GraphQL federation compatibility."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 :) |
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.
Hey @csechrist - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding documentation for the new 'directives' parameter in the type mapping function. This will help users understand how to utilize this new feature effectively.
- Please review and update the PR checklist. Ensuring all applicable items are checked will help maintain the project's quality standards.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Thank you for your contribution! I've reviewed your code and left one comment, please take a look when you can.
Additionally, it would be great if you could create new tests to ensure this solution remains stable in future updates.
I also believe this change needs to be added on our documentation. While I know our docs aren't perfect, adding a note to the README in Markdown will works for now. I think it could go right before the Limitations
section, what do you think?
Thanks again!
Sounds great! I will get tests added in and get the documentation updated this week! |
Hi, @csechrist , any updates? |
Description
Allow for directives to be added to the SQLAlchemy type mapper. This enables us to use it with GraphQL federation more effectively
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Enhance the SQLAlchemy type mapper to support directives, facilitating better integration with GraphQL federation, and update pre-commit hooks to their latest versions.
Enhancements:
Build: