Skip to content

Added sort support #101

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

Merged
merged 11 commits into from
Jun 22, 2018
Merged

Conversation

CaselIT
Copy link
Contributor

@CaselIT CaselIT commented Dec 16, 2017

Add support for sorting the return values of a SQLAlchemyConnectionField

The sort argument of SQLAlchemyConnectionField supports sorting over a single over multiple field.

In the utils file I've added

  • the function sort_enum_for_model to create a graphene Enum with all be possible field and order combination for a particular sqlalchemy model as suggested by @Cito
  • the function sort_argument_for_model to automacally create the enum and return the attribute to use in the connection field

The test and documentation are still missing, because first I wanted to get some feedback about the implementation.

Regarding the value of the sort enumerator at first I tried to set the values of the enum to the ascenting or descenting operators of sqlalchemy simplify the logic of the get_query but that results in an error when graphql builds the ast graph, so I reverted to use a list column_name, operator that is less than optimal since every time it needs to query the model to create the operators.

Thanks to @Cito to the input in #40
Closes #40

@coveralls
Copy link

coveralls commented Dec 16, 2017

Coverage Status

Coverage decreased (-6.5%) to 84.424% when pulling b192e78 on CaselIT:sortable_field into 4827ce2 on graphql-python:master.

@CaselIT CaselIT changed the title Added sortable support Added sort support Dec 16, 2017
@coveralls
Copy link

coveralls commented Dec 16, 2017

Coverage Status

Coverage decreased (-6.7%) to 84.308% when pulling d0337ba on CaselIT:sortable_field into 4827ce2 on graphql-python:master.

@CaselIT
Copy link
Contributor Author

CaselIT commented Dec 16, 2017

Regarding the value of the sort enumerator at first I tried to set the values of the enum to the ascenting or descenting operators of sqlalchemy simplify the logic of the get_query but that results in an error when graphql builds the ast graph, so I reverted to use a list column_name, operator that is less than optimal since every time it needs to query the model to create the operators.

I've changed the implementation to use a subclass of string instead of the list, so that the ast graph can be created successfully and there is no need to query the model every time.

The sort enum value is now a subclass of str that cat store the sort operator and be treated as string when creating the ast graph
@coveralls
Copy link

coveralls commented Dec 16, 2017

Coverage Status

Coverage decreased (-6.7%) to 84.308% when pulling 2fd5203 on CaselIT:sortable_field into 4827ce2 on graphql-python:master.

@Cito
Copy link
Member

Cito commented Dec 16, 2017

Nice, thank you.

Maybe, as I've already commented in #40, the sort argument should be added by default to the SQLAlchemyConnectionField, so that you don't need to specify it manually. That would be similar to how a Graphene ConnectionField comes with first, last arguments by default, which also don't need to added manually.

@CaselIT
Copy link
Contributor Author

CaselIT commented Dec 16, 2017

Good idea. I'll add it.
Do you think it should have a default value when added by default?

@Cito
Copy link
Member

Cito commented Dec 17, 2017

For an SQLAlchemyConnectionField it probably makes sense to use the id as default value for the sort argument so that you always get a reproducable order.

@CaselIT
Copy link
Contributor Author

CaselIT commented Dec 17, 2017

SQLAlchemyConnectionField now adds the sort attribute by default, with a default sort over the primary key(s). To disable this field pass to the constructor sort=None

@coveralls
Copy link

coveralls commented Dec 17, 2017

Coverage Status

Coverage increased (+1.4%) to 92.486% when pulling fb6913b on CaselIT:sortable_field into 8cb52a1 on graphql-python:master.

@exit99
Copy link

exit99 commented Dec 28, 2017

This going to get merged?

@CaselIT
Copy link
Contributor Author

CaselIT commented Dec 28, 2017

I hope so.
@syrusakbary could you review this PR?
Thanks

@CaselIT
Copy link
Contributor Author

CaselIT commented Jan 29, 2018

@syrusakbary see #104 (comment)

@YehonatanVonage
Copy link

Hi,
Any updates about this PR?

@CaselIT
Copy link
Contributor Author

CaselIT commented Mar 13, 2018

On my part I wanted a feedback from @syrusakbary about the implementation before writing the tests.

@millicentachieng
Copy link

@syrusakbary We are waiting on you to review the PR

@syrusakbary
Copy link
Member

Sorry for the time taken on reviewing this PR, and thanks for the ping.
It looks good, once it have tests related to the new functionality I'll merge :)

@CaselIT
Copy link
Contributor Author

CaselIT commented Jun 4, 2018

Thanks for the review @syrusakbary
I'll work on adding the tests and ping you once they are done

@CaselIT
Copy link
Contributor Author

CaselIT commented Jun 21, 2018

@syrusakbary I think it's ready for a review
I've added the tests
Updated the implementation to support graphene v2
Added a documentation example and updated some other documentation to be compatible with v2

Let me know what you think

@syrusakbary
Copy link
Member

It looks great! You did a great job 👏

Merging

@syrusakbary syrusakbary merged commit 5346496 into graphql-python:master Jun 22, 2018
@CaselIT
Copy link
Contributor Author

CaselIT commented Jun 22, 2018

Thank you for merging it!

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.

7 participants