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

added contains express support #395

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iakashpatel
Copy link

@iakashpatel iakashpatel commented May 26, 2023

Why this PR?

Recently I started using https://sqlalchemy-utils.readthedocs.io/en/latest/_modules/sqlalchemy_utils/types/scalar_list.html to store array of enum.

for which I need to filter table with specific value inside array of enum (TEXT/BLOB at low level) which is feasible using contains expression.

What is changed/added ?

  • Added contains expression support for sqlalchemy and testcases to support it.
  • Updated Doc/Example

PS: 🌟 My first open-source contribution :)

@netlify
Copy link

netlify bot commented May 26, 2023

Deploy Preview for fastapi-filter ready!

Name Link
🔨 Latest commit 0cab115
🔍 Latest deploy log https://app.netlify.com/sites/fastapi-filter/deploys/64703a823aa568000835e50c
😎 Deploy Preview https://deploy-preview-395--fastapi-filter.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Owner

@arthurio arthurio left a comment

Choose a reason for hiding this comment

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

Hey @iakashpatel 👋🏻
Thanks for the contribution, this seems useful for more that just the scalar list type, thanks for adding it 👍🏻
However, the tests you have updated don't test the actual feature you are looking for. Could you update the tests as mentioned in the comments?

Comment on lines -92 to +97
[{"is_individual": True, "bogus_filter": "bad"}, status.HTTP_422_UNPROCESSABLE_ENTITY],
[{"is_individual": True, "bogus_filter": "bad"},
status.HTTP_422_UNPROCESSABLE_ENTITY],
Copy link
Owner

Choose a reason for hiding this comment

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

This should be reverted.

@@ -265,6 +265,7 @@ class UserFilter(Filter): # type: ignore[misc, valid-type]
name: Optional[str]
name__neq: Optional[str]
name__like: Optional[str]
name__contains: Optional[str]
Copy link
Owner

Choose a reason for hiding this comment

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

Here name is not a list of enum so I'm not sure we are testing exactly what you are looking for... Could you add another enum list field instead and test against it?

@codecov
Copy link

codecov bot commented May 29, 2023

Codecov Report

Merging #395 (0cab115) into main (64c93b9) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #395   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          180       180           
=========================================
  Hits           180       180           
Impacted Files Coverage Δ
fastapi_filter/contrib/sqlalchemy/filter.py 100.00% <ø> (ø)

@raphodn
Copy link

raphodn commented Feb 27, 2024

Hi @iakashpatel , I'm interested in the same feature, and was wondering if you still planned to work on this PR ? If not I might consider opening one myself

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.

3 participants