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

Make NewFirestoreRepository receive the collection name #29

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marcoshuck
Copy link
Collaborator

No description provided.

@marcoshuck marcoshuck self-assigned this Jun 1, 2023
@AlejoAsd
Copy link
Contributor

AlejoAsd commented Jun 1, 2023

Isn't the collection name part of the Model passed in as a generic parameter?

@marcoshuck
Copy link
Collaborator Author

Isn't the collection name part of the Model passed in as a generic parameter?

Yes, but what if you ever want to use a different collection name with the same model? I think that when we designed the Model interface, we didn't take that use case into account.

However, I'm still running some tests with the e2e suite for listing solutions, will let you know if I end up needing a review on this.

@AlejoAsd
Copy link
Contributor

AlejoAsd commented Jun 1, 2023

In that case I would suggest creating a Model implementation that can you can specify a collection name for, and have the TableName() (which admittedly is a very SQL-centric name) return the collection name for it.

In short: this is a problem to solve at the Model level, not at the repository level.

@marcoshuck
Copy link
Collaborator Author

In that case I would suggest creating a Model implementation that can you can specify a collection name for, and have the TableName() (which admittedly is a very SQL-centric name) return the collection name for it.

In short: this is a problem to solve at the Model level, not at the repository level.

It's not that easy, because we're using the model as a generic T type in the Firestore repository, no place where to initialize it.

@AlejoAsd
Copy link
Contributor

AlejoAsd commented Jun 1, 2023

Great point. This is one of the shortcomings of generics unfortunately. I'm inclined to remove the generic part and just use reflection instead of a mixed approach as we need to interact with the type itself.

Let's go ahead with this approach for now.

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