Skip to content
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

Integrating support for graphene-sqlalchemy-filter into code base #335

Open
palisadoes opened this issue Mar 29, 2022 · 10 comments
Open

Integrating support for graphene-sqlalchemy-filter into code base #335

palisadoes opened this issue Mar 29, 2022 · 10 comments

Comments

@palisadoes
Copy link
Collaborator

As there is limited filtering with graphene-sqlalchemy, I have been using graphene-sqlalchemy-filter which has many stock filters included. Unfortunately the developer seems to have lost interest in the code base and a critical PR to add support for Graphene 3.0 has been unmerged for six months.

I have tried contacting the developer a few times without response.

As the filtering code for the add-on is contained in the graphene-sqlalchemy-filter package's two files, I propose merging this code into graphene-sqlalchemy.

Would this be OK?

@erikwrede
Copy link
Member

I just followed your link from the filter repo, so I thought I might leave a reply here as well :)

While in general I support the idea of adding filters to graphene-sqa, and like the approach that the author has taken, I think a simple merge won't do it. The filters should be deeply integrated into this repo to allow for even more features, so this has to be planned. But I think using this library as inspiration would be a great start.
Maybe we can work something out together? Or maybe one of the maintainers of this repo has any supporting or opposing ideas to consider before starting. It would be sad to see yet another merge request get stuck unapproved for a long time.

@palisadoes
Copy link
Collaborator Author

Thanks. My python skills are good, but not great. However, I'd be willing to learn. @cooldudemcgeexl would you be interested in a collaboration?

@palisadoes palisadoes mentioned this issue Mar 30, 2022
@palisadoes
Copy link
Collaborator Author

We have an interested party in adding the filtering syntax.

@erikwrede
Copy link
Member

Sounds great!

I think the current external library is great but still has some places where the design lacks flexibility.
Maybe we should find some interested users and collect their filtering requirements here as a start?

@palisadoes
Copy link
Collaborator Author

palisadoes commented Mar 31, 2022

@sabard and @PaulSchweizer are working on a requirements Google Doc to help determine the expected time and cost. Join us on the #graphene-sqlalchemy-filter channel on the Graphene Slack organization. The link can be found here https://github.com/graphql-python/graphene

The starting point would be to mimic the API interface that the graphene-sqlalchemy-filter currently provides. Let us know what flexibility you'd like to see and add it to the doc.

@erikwrede
Copy link
Member

Implementation is in progress. The main PR will be #357. Please join us on Slack #graphene-sqlalchemy-filter if you would like to contribute or have anything to add!

@erikwrede erikwrede mentioned this issue Aug 12, 2022
9 tasks
@erikwrede erikwrede pinned this issue Aug 12, 2022
@erikwrede erikwrede added this to the 3.0 milestone Aug 15, 2022
@polgfred
Copy link
Contributor

Just bumping this to see where things are. I'm also using a fork of graphene-sqlalchemy-filter with some patches, including a small change to address the latest conversion refactor, but would love to see filters fully integrated and working. Happy to pitch in if needed!

@palisadoes
Copy link
Collaborator Author

@polgfred Coincidentally we had a meeting yesterday where we decided it's time to start alpha testing this integration.

  1. Use the add-filters branch: https://github.com/graphql-python/graphene-sqlalchemy/tree/add-filters
  2. We don't have documentation yet, but should have it soon. You'll need to use the test cases as the interim documentation: https://github.com/graphql-python/graphene-sqlalchemy/blob/add-filters/graphene_sqlalchemy/tests/test_filters.py
  3. You'll notice that the original filter uses the filters keyword, we are using the filter keyword. The rationale for this is so that both methodologies can work side by side as people transition (assuming the package dependencies are frozen)

@erikwrede, @sabard can you provide the link to the original syntax guide we are using? That will help greatly until we get the documentation updated.

@erikwrede
Copy link
Member

@polgfred cadu should have the syntax guide via google docs. If you want access too, please send me your email address via Slack 🙂

@palisadoes
Copy link
Collaborator Author

@polgfred These are the syntax guides we used as the templates for the work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants