-
-
Notifications
You must be signed in to change notification settings - Fork 34
Option to always "use_list" and not make relay Connections #257
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduces an always_use_list flag to StrawberrySQLAlchemyMapper allowing users to default to list-based relationship fields instead of relay connections. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/strawberry_sqlalchemy_mapper/mapper.py:678` </location>
<code_context>
def connection_resolver_for(
self, relationship: RelationshipProperty, use_list=False
) -> Callable[..., Awaitable[Any]]:
"""
Return an async field resolver for the given relationship that
returns a Connection instead of an array of objects.
This resolver also handles pagination arguments (first, after, last, before) that are
passed from the GraphQL query to the database query.
"""
relationship_resolver = self.relationship_resolver_for(relationship)
if relationship.uselist and not (use_list or self.always_use_list):
return self.make_connection_wrapper_resolver(
relationship_resolver,
relationship,
)
else:
return relationship_resolver
</code_context>
<issue_to_address>
**suggestion (code-quality):** Simplify logical expression using De Morgan identities ([`de-morgan`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/de-morgan/))
```suggestion
if relationship.uselist and not use_list and not self.always_use_list:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Thanks for adding the Here's a preview of the changelog: Added a new optional constructor parameter to always use lists instead of relay Connections for relationships. Defaults to False, maintaining current functionality. If set to True, all relationships will be handled as lists. Example: |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
Thanks for your contribuition, @gabor-lbl , I hope to review it in this weekend! |
Ckk3
left a comment
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.
Thanks for your contribution, @gabor-lbl
If you can't find time to do the comments please tell me that I will continue from here.
| if relationship.uselist: | ||
| # Use list if excluding relay pagination | ||
| if use_list: | ||
| if use_list or self.always_use_list: |
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.
todo (blocking)
I think we should create tests to validate this changes, what do you think @gabor-lbl ?
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.
I agree. When I run tox I get an error though:
❯ tox
.pkg: _optional_hooks> python /Users/gtorok/dev/strawberry-sqlalchemy/aaa/lib/python3.9/site-packages/pyproject_api/_backend.py True poetry.core.masonry.api
.pkg: get_requires_for_build_sdist> python /Users/gtorok/dev/strawberry-sqlalchemy/aaa/lib/python3.9/site-packages/pyproject_api/_backend.py True poetry.core.masonry.api
.pkg: get_requires_for_build_wheel> python /Users/gtorok/dev/strawberry-sqlalchemy/aaa/lib/python3.9/site-packages/pyproject_api/_backend.py True poetry.core.masonry.api
.pkg: prepare_metadata_for_build_wheel> python /Users/gtorok/dev/strawberry-sqlalchemy/aaa/lib/python3.9/site-packages/pyproject_api/_backend.py True poetry.core.masonry.api
.pkg: build_sdist> python /Users/gtorok/dev/strawberry-sqlalchemy/aaa/lib/python3.9/site-packages/pyproject_api/_backend.py True poetry.core.masonry.api
default: install_package> python -I -m pip install --force-reinstall --no-deps /Users/gtorok/dev/strawberry-sqlalchemy/.tox/.tmp/package/6/strawberry_sqlalchemy_mapper-0.7.0.tar.gz
default: commands[0]> pytest
default: failed with pytest is not allowed, use allowlist_externals to allow it
.pkg: _exit> python /Users/gtorok/dev/strawberry-sqlalchemy/aaa/lib/python3.9/site-packages/pyproject_api/_backend.py True poetry.core.masonry.api
default: FAIL code 1 (1.53 seconds)
evaluation failed :( (1.54 seconds)
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.
I've never used tox or poetry before so please let me know if I'm doing something wrong.
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.
@gabor-lbl Dont need to bother with this, if it runs with pytest in your machine you can just push the changes that our CI will setup and run the tests again using nox in every python version we support 😉
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.
What do I need to install to run pytest? I ran pytest on its own and it needed testing.postgres. I installed that but it still won't run:
❯ pytest
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --emoji --mypy-ini-file=mypy.ini
inifile: /Users/gtorok/dev/strawberry-sqlalchemy/pyproject.toml
rootdir: /Users/gtorok/dev/strawberry-sqlalchemy
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.
@gabor-lbl Oh you need to have a postgres soo it can create the database.
I recommend you use a devcontainer since it automatically runs a postgres container
, we have one in our project just open your vscode, press f1 and select "Reopen in Container"

If you prefer, we can also talk in strawberry discord server, you can find me there as @gustavom0ta
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.
Ok thanks - I added a new test to check the always_use_list param.
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.
Thanks @gabor-lbl , I'll review it later
… instead of None. Updated README.
| if relationship.uselist: | ||
| # Use list if excluding relay pagination | ||
| if use_list: | ||
| if use_list or self.always_use_list: |
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.
@gabor-lbl Dont need to bother with this, if it runs with pytest in your machine you can just push the changes that our CI will setup and run the tests again using nox in every python version we support 😉
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #257 +/- ##
==========================================
+ Coverage 92.31% 92.39% +0.08%
==========================================
Files 19 19
Lines 2395 2421 +26
Branches 188 188
==========================================
+ Hits 2211 2237 +26
Misses 98 98
Partials 86 86 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #257 will not alter performanceComparing Summary
|

Description
I added an optional constructor parameter to always use lists for relationships and not make relay Connections. This way users who don't use Connections don't have to add
__use_list__to every model.Types of Changes
Issues Fixed or Closed by This PR
Checklist
toxto run... will continue to look into this)(I'm using it in my project.)
Summary by Sourcery
Add optional always_use_list parameter to StrawberrySQLAlchemyMapper to default relationship fields to lists and bypass Relay Connection creation.
New Features:
Enhancements: