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

Automatic sort argument for SQLAlchemyInterface #400

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gbunkoczi
Copy link

When using SQLAlchemyConnectionField with a SQLAlchemyInterface to get a polymorphic connection, one has to set the sort argument to None to disable automatic sort enum generation. However, the type check in register_sort_enum is too strict, and being an instance of SQLAlchemyBase seems to be sufficient for the code to work.

Tests run with Python 3.8 and 3.12.

 - change required type to SQLAlchemyBase in Registry.register_sort_enum
- update register_sort_enum test
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9c2bc84) 94.74% compared to head (f5e6d32) 94.74%.

❗ Current head f5e6d32 differs from pull request most recent head 3ade1e6. Consider uploading reports for the commit 3ade1e6 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #400   +/-   ##
=======================================
  Coverage   94.74%   94.74%           
=======================================
  Files          10       10           
  Lines        1333     1333           
=======================================
  Hits         1263     1263           
  Misses         70       70           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- allow SQLALchemyConnectionField.connection for all SQLAlchemyBase types
Copy link
Member

@erikwrede erikwrede left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the fix. Can you please add a regression test with your described connection case? I don't have the exact repro so it would be best if you used yours. This test is important so we don't accidentally re-introduce that PR. After that, this will be good to merge! 🙂

- test to check that sort argument is generated automatically for a SQLALchemyConnectionField that uses a SQLALchemyInterface.connection
- test for relationship filtering a model using a relationship to a table with joined table inheritance
@gbunkoczi
Copy link
Author

Apologies for the delay. I put some tests in place.

Somewhat tangentially, there seems to be more cases of "issubclass(obj_type, SQLAlchemyObjectType)" tests in the codebase, but since these were not triggered so far, I have not changed them. In addition, "- obj_type : SQLAlchemyObjectType" appears multiple times in function docstrings where " -obj_type: SQLALchemyBase" would be more appropriate, e.g.

- obj_type : SQLAlchemyObjectType

- obj_type : SQLAlchemyObjectType

(these call registry.register_sort_enum, which has been switched to test SQLALchemyBase). I am happy to correct these as part of this PR if this would help.

@erikwrede
Copy link
Member

@gbunkoczi Awesome! if you have time, please feel free to go ahead. Thanks for your effort! 😊

- change instances of SQLAlchemyObjectType to SQLAlchemyBase as appropriate
- update tests
@gbunkoczi
Copy link
Author

The checks have all been changed. I tried each call with a SQLAlchemyInterface instance, and all worked as expected. The docstrings were also updated when appropriate.

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.

2 participants