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

PypikaRepository can't be extended to support other db types #104

Closed
zznty opened this issue Jul 27, 2022 · 3 comments
Closed

PypikaRepository can't be extended to support other db types #104

zznty opened this issue Jul 27, 2022 · 3 comments

Comments

@zznty
Copy link
Contributor

zznty commented Jul 27, 2022

Description

Currently we have PypikaRepository supporting just SQLlite with promised support for other types (MySQL, PostgreSQL),
but to add this support we will need to create dedicated repository classes such as PostgreSqlRepository, because combining everything into one class will result in tons of conditions and unnecessary complication of code.

Possible Solutions

  • Rename current version of PypikaRepository to SQLliteRepository, but can result in code duplication for generic pypika operations.
  • Extract generic methods for pypika into PypikaRepository and create SQLliteRepository with SQLlite-related code
@lyz-code
Copy link
Owner

lyz-code commented Aug 1, 2022

Hi @zznty thanks for taking the time to fill up an issue and sorry for the delay answering. Why do you think adding support for Mysql and Postgres will require to create tons of conditions and unnecessary complication of code? The only reason of using Pypika was that is a query builder that does all the translations required for the different databases behind the scenes.

@zznty
Copy link
Contributor Author

zznty commented Aug 3, 2022

for example you have multiple code fragments that wont work with other db types and will require something like this

if db_type == "sqllite":
 ...
elif db_type == "mysql":
 ...

@lyz-code
Copy link
Owner

lyz-code commented Aug 4, 2022

I see what you mean, so far I've only tested that it works with SQLite. Once we implement the tests of the other databases, we'll see if the changes are big enough to justify the need of another repository.

If you want to contribute this feature, I'd say that you first start tweaking the PypikaRepository to make the other database work, and if we need we'll refactor it in a new class.

I'm closing this issue as I feel it's a duplicated of #1, specifically it's the discussion of it's implementation. If you want, we can continue the discussion there

@lyz-code lyz-code closed this as completed Aug 4, 2022
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

No branches or pull requests

2 participants