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

Initial generic rethinkdb store #539

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Initial generic rethinkdb store #539

wants to merge 10 commits into from

Conversation

majst01
Copy link
Contributor

@majst01 majst01 commented Jun 17, 2024

Just to see how this would feel

@ulrichSchreiner
Copy link
Contributor

ulrichSchreiner commented Jun 17, 2024

You use metal.Entity as a constraint of your type parameter. But the whole store only works, if the type is a pointer receiver for your constraint; so you always have to use pointer-types as type parameters: *metal.IP, *metal.Machine , ...

This makes the API a little akward: Take the Create method:

func (rs *rethinkStore[E]) Create(ctx context.Context, e E) error

If you only look at the signature you do not see that the last parameter has to be a pointer to a type. Imho it is also not a very good style to have such an output parameter. Why not have a signature like

func (rs *rethinkStore[E]) Create(ctx context.Context) (*E, error)

In this case it is clear that you return a pointer to your type parameter. But this requires your type constraint to be more tricky: I think you need two type parameters. One for the type without a constraint and the second with our metal.Entity interface which should also contain a type parameter which is a simple a pointer-receiver to the type parameter. so you can alwas create a pointer-receiver out of your type parameter to call Set... methods (which only work on pointer receivers) or use the new function (which also only works with pointers).

The type parameters proposal has a good example for this problem.

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