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

feat: repository enhancements and MySQL SQLAlchemy support #1345

Merged
merged 78 commits into from Apr 20, 2023
Merged

feat: repository enhancements and MySQL SQLAlchemy support #1345

merged 78 commits into from Apr 20, 2023

Conversation

cofin
Copy link
Member

@cofin cofin commented Mar 18, 2023

This PR adds the following additional features:

  • adds support and tests for MySQL using the asyncmy drivers.
  • Basic repository docs. A more comprehensive usage guide will be completed when the Service and DTO patterns have been merged
  • get_or_create optionally accepts a match_fields argument. This lets you lookup a model using a subset of the kwargs you've provided. If the remaining kwargs are different from the retrieved model's stored values, an update is performed.
  • Adds OrderBy and SearchFilter filter types. OrderBy appends the ORDER BY clause to a statement and SearchFilter adds WHERE LIKE .. clause. It optionally has a case insensitive override.
  • Renamed base_select to statement.

PR Checklist

  • Have you followed the guidelines in CONTRIBUTING.rst?
  • Have you got 100% test coverage on new code?
  • Have you updated the prose documentation?
  • Have you updated the reference documentation?

@cofin cofin requested a review from a team as a code owner March 18, 2023 19:47
@provinzkraut
Copy link
Member

Since these database backed tests are quite slow, can we maybe add a pytest marker to skip them?

Copy link
Contributor

@Goldziher Goldziher left a comment

Choose a reason for hiding this comment

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

can we run the DB tests only on main? or maybe using a schedule (github action cron basically).

@provinzkraut
Copy link
Member

can we run the DB tests only on main? or maybe using a schedule (github action cron basically).

Hmm. I think we generally do want to run those in our CI and locally (maybe optionally). It's not like they take ages to run (the asyncpg tests take 5 seconds once the container is set up). That's not too bad on its own, but if we start adding many other dbs, it will somewhat add up. For me, just having a marker to be able to skip them if I want would be good enough. For the CI, it definitely doesn't matter since we have steps that take significantly longer than our whole test suite.

@Goldziher
Copy link
Contributor

can we run the DB tests only on main? or maybe using a schedule (github action cron basically).

Hmm. I think we generally do want to run those in our CI and locally (maybe optionally). It's not like they take ages to run (the asyncpg tests take 5 seconds once the container is set up). That's not too bad on its own, but if we start adding many other dbs, it will somewhat add up. For me, just having a marker to be able to skip them if I want would be good enough. For the CI, it definitely doesn't matter since we have steps that take significantly longer than our whole test suite.

ok

@provinzkraut
Copy link
Member

@cofin is this currently blocked by something?

@cofin
Copy link
Member Author

cofin commented Apr 4, 2023

No, it's not blocked by anything, and I need to wrap this up. Based on the last status, I need to add the Pytest marker and merge the latest changes.

@provinzkraut
Copy link
Member

No, it's not blocked by anything, and I need to wrap this up. Based on the last status, I need to add the Pytest marker and merge the latest changes.

Okay, cool, just checking in!

Cody Fincher added 6 commits April 7, 2023 14:39
- rename `base_select` to `statement` and swap order
- add `OrderBy` FilterType
- add `match_fields` to `get_or_create` so that you can search for a model with a limited set of values from your `kwargs` instead of all fields
- add `SearchFilter` FilterType to append a `where field like `%..%` filter type.
@cofin
Copy link
Member Author

cofin commented Apr 8, 2023

I've been having issues with the asyncmy library, and it looks like there's an existing issue that references the problem: long2ice/asyncmy#39

@cofin
Copy link
Member Author

cofin commented Apr 8, 2023

I've excluded these mysql tests by default. I can do the same with the asyncpg tests if that makes sense?

@cofin cofin changed the title feat: introduces mysql tests using asyncmy feat: repository documentation and updates and asyncmy tests Apr 8, 2023
@cofin cofin marked this pull request as draft April 8, 2023 22:53
@peterschutt
Copy link
Contributor

I've excluded these mysql tests by default. I can de the same with the asyncpg tests if that makes sense?

I.e., exclude from the general CI tests?

@cofin cofin requested a review from JacobCoffee as a code owner April 18, 2023 22:54
@cofin cofin requested a review from provinzkraut April 18, 2023 23:03
@cofin cofin changed the title feat: repository documentation and updates and asyncmy tests feat: repository enhancements and MySQL SQLAlchemy support Apr 18, 2023
.github/workflows/test.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
@cofin cofin merged commit 0241d90 into litestar-org:main Apr 20, 2023
11 checks passed
@cofin cofin deleted the mysql-tests branch April 25, 2023 17:54
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.

None yet

5 participants