strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
MIT License
391 stars 115 forks source link

Filter & Order ovehaul #478

Closed Kitefiko closed 3 months ago

Kitefiko commented 4 months ago

Description

Filters

Order

Types of Changes

Issues Fixed or Closed by This PR

Checklist

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 97.33728% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 88.46%. Comparing base (17720f3) to head (1f5fdc8). Report is 2 commits behind head on main.

Files Patch % Lines
strawberry_django/fields/filter_order.py 94.44% 6 Missing :warning:
strawberry_django/exceptions.py 93.54% 2 Missing :warning:
strawberry_django/fields/filter_types.py 98.36% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #478 +/- ## ========================================== + Coverage 87.91% 88.46% +0.54% ========================================== Files 37 40 +3 Lines 3170 3381 +211 ========================================== + Hits 2787 2991 +204 - Misses 383 390 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bellini666 commented 4 months ago

hey @Kitefiko ,

Let me know once this is finished and you want me to review it :)

Kitefiko commented 3 months ago

Hey, I think it is pretty much done, so review away :)

There is one decision to be made that I mentioned in original issue:

bellini666 commented 3 months ago

Hey, I think it is pretty much done, so review away :)

There is one decision to be made that I mentioned in original issue:

  • We could ignore null even for exact, iexact, make user use isnull (I would argue, this is better anyway). This was mentioned here - lookups doesn't accept null #459

Yes, I agree that we should be used isnull here

Kitefiko commented 3 months ago

Hi,

resolved all raised issues. Additionally made all lookups ignore null by default.

Would be nice to have some pylance pre-commit hook for typing, if there is one?

bellini666 commented 3 months ago

Hi,

resolved all raised issues. Additionally made all lookups ignore null by default.

Awesome! Will take a look at the end of the day for me here :)

Would be nice to have some pylance pre-commit hook for typing, if there is one?

Hrm, maybe we can do something like this: https://github.com/microsoft/pyright/blob/main/docs/ci-integration.md#running-pyright-as-a-pre-commit-hook

The reason why I didn't add that yet is because pyright takes a lot more time to run than other general linters

Kitefiko commented 3 months ago

Hrm, maybe we can do something like this: https://github.com/microsoft/pyright/blob/main/docs/ci-integration.md#running-pyright-as-a-pre-commit-hook

The reason why I didn't add that yet is because pyright takes a lot more time to run than other general linters

Makes sense, maybe adding it to pre-commit (for developers), skip it during github pre-commit & then run it explicitly same as now?