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

Move Repository SQL implementation into own package #23

Merged
merged 2 commits into from
May 18, 2023

Conversation

AlejoAsd
Copy link
Contributor

@AlejoAsd AlejoAsd commented May 18, 2023

Closes #21.

The repository package provides a generic Repository interface and a number of implementations for it. Up until this point, there had only ever been a SQL implementation, and it was defined at the repository package level. Because both interface and implementations are all contained under a single namespace, other implementations may run into namespace collision issues with the SQL implementation.

This PR moves the SQL implementation into its own package to avoid collision issues.

The new packages structure looks like this:

repository    Contains generic definitions such as the `Repository` interface and related types.
└── sql       SQL Repository implementation.

New implementation should be defined in their own package inside the repository package.

Signed-off-by: Alejo Carballude <alejocarballude@gmail.com>
Signed-off-by: Alejo Carballude <alejocarballude@gmail.com>
@AlejoAsd AlejoAsd self-assigned this May 18, 2023
@AlejoAsd AlejoAsd requested a review from marcoshuck May 18, 2023 20:15
@marcoshuck
Copy link
Collaborator

marcoshuck commented May 18, 2023

I like how the naming convention looks like, but I have a concern: We don't use any sql package (gorm does it for us), so it works for this case, but when we start adding new implementations (firestore, etc), relying on the same name will create collisions. This is an issue we've solved in the past by creating package aliases, I don't think this should be our default approach, but the exception to the rule.

Why don't we create an option builder that configures a set of options based on a certain repository type? This component can live in the repository package, avoiding the problem I mentioned before. Firestore uses the concept of Query and it provides a builder to parameterize queries.

@AlejoAsd
Copy link
Contributor Author

Thanks for the comments.

I like how the naming convention looks like, but I have a concern: We don't use any sql package (gorm does it for us), so it works for this case, but when we start adding new implementations (firestore, etc), relying on the same name will create collisions. This is an issue we've solved in the past by creating package aliases, I don't think this should be our default approach, but the exception to the rule.

I used the sql package name because I didn't want to imply any connection to the implementation. The sql Repository implementation is using gorm today, but it might not always be the case. Imports downstream should not change based on the internal implementation of the SQL repository.

I don't think we are likely to ever import sql directly, but I do agree that it will probably require aliases to avoid collisions or simply make it less confusing to people reading code.

Why don't we create an option builder/factory that creates an option based on a certain repository type? This component can live in the repository package, avoiding the problem I mentioned before. Firestore uses the concept of Query and it provides a builder to parameterize queries.

Mainly because options are very specific to the implementation. I expect all SQL implementations to use Options in the sql package.

If we ever determine that the Firestore implementation shares options with other implementations, then we can consider renaming the firestore or firebase package to whatever the common denominator is (e.g. nosql). For the time being, I'm trying to minimize the work we do upfront and address this once it is an issue, similar to how we now moved the SQL implementation into its own package.

@AlejoAsd AlejoAsd marked this pull request as ready for review May 18, 2023 21:06
@marcoshuck
Copy link
Collaborator

Mainly because options are very specific to the implementation. I expect all SQL implementations to use Options in the sql package.

I don't think this is an issue for having a generic query builder, and passing some sort of converter once calling the Build method, reducing the amount of duplicated code and/or package name collisions

For the time being, I'm trying to minimize the work we do upfront and address this once it is an issue, similar to how we now moved the SQL implementation into its own package.

Agreed on that, this PR looks good, and we shouldn't address the issue I'm raising right now.

@AlejoAsd
Copy link
Contributor Author

I don't think this is an issue for having a generic query builder, and passing some sort of converter once calling the Build method, reducing the amount of duplicated code and/or package name collisions

I'm not sure I'm following. Care to give an example of what you're envisioning?

Agreed on that, this PR looks good, and we shouldn't address the issue I'm raising right now.

+1 Appreciate the discussion!

@AlejoAsd AlejoAsd merged commit 5bba272 into main May 18, 2023
@AlejoAsd AlejoAsd deleted the feature/repository_sql_package branch May 18, 2023 21:13
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.

Move implementation-specific Repository Options to packages
2 participants