Skip to content

Add batching params #260

Merged
jnak merged 4 commits into
graphql-python:masterfrom
jnak:batch-sql-4
Feb 12, 2020
Merged

Add batching params #260
jnak merged 4 commits into
graphql-python:masterfrom
jnak:batch-sql-4

Conversation

@jnak
Copy link
Copy Markdown
Collaborator

@jnak jnak commented Jan 24, 2020

Add parameters to toggle batching on or off. This can be configured at 2 levels:

  • we can configure all the fields of a type at once via SQLAlchemyObjectType.meta.batching
  • or we can specify it for a specific field via ORMfield.batching. This trumps SQLAlchemyObjectType.meta.batching.

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 24, 2020

Coverage Status

Coverage increased (+0.05%) to 97.578% when pulling 7003e60 on jnak:batch-sql-4 into 7a48d3d on graphql-python:master.

Copy link
Copy Markdown
Collaborator

@Nabellaleen Nabellaleen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've copy the comments I've done on your fork PR, to be able to follow easily the answers here ;)

Comment thread graphene_sqlalchemy/converter.py Outdated
Comment thread graphene_sqlalchemy/converter.py Outdated
Comment thread graphene_sqlalchemy/converter.py Outdated
Comment thread graphene_sqlalchemy/converter.py Outdated
resolver = get_custom_resolver(obj_type, orm_field_name)
if resolver is None:
resolver = get_batch_resolver(relationship_prop) if batching_ else \
get_attr_resolver(obj_type, relationship_prop.key)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
get_attr_resolver(obj_type, relationship_prop.key)
elif resolver is None:
resolver = get_attr_resolver(obj_type, relationship_prop.key)

Comment thread graphene_sqlalchemy/converter.py Outdated
katy-sadowski
katy-sadowski previously approved these changes Jan 27, 2020
Copy link
Copy Markdown

@katy-sadowski katy-sadowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a test to check approve rights.

@jnak
Copy link
Copy Markdown
Collaborator Author

jnak commented Jan 27, 2020

@Nabellaleen Ignore @katy-sadowski approval - I asked her to do this so I can tests different permission settings.

fix tests
Nabellaleen
Nabellaleen previously approved these changes Jan 29, 2020
@jnak jnak merged commit 17d535e into graphql-python:master Feb 12, 2020
@himat
Copy link
Copy Markdown

himat commented Aug 28, 2020

Was the default batching true or false?

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.

5 participants