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

Add common helpers to gRPC client package #554

Closed
zepatrik opened this issue Apr 21, 2021 · 2 comments · Fixed by #657
Closed

Add common helpers to gRPC client package #554

zepatrik opened this issue Apr 21, 2021 · 2 comments · Fixed by #657
Labels
corp/m8 Up for M8 at Ory Corp.

Comments

@zepatrik
Copy link
Member

Is your feature request related to a problem? Please describe.

Because of the sometimes quite nested structure of messages, there are some pieces of code I keep copying, e.g.

func InsertRelationTuples(rts []*acl.RelationTuples) []*acl.RelationTupleDelta {
	deltas := make([]*acl.RelationTupleDelta, len(rts))
	for i := range rts {
		deltas[i] = &acl.RelationTupleDelta{
			Action:        acl.RelationTupleDelta_INSERT,
			RelationTuple: rts[i],
		}
	}
	return deltas
}

Describe the solution you'd like

It would be nice to have them added to the generated acl package as helpers.

Describe alternatives you've considered

A separate helper package outside of internal that only depends on acl.

Additional context

Is there any downside of adding such helpers to the generated acl package @robinbraemer ?

@robinbraemer
Copy link
Contributor

robinbraemer commented Apr 21, 2021

Mixing generated code files and written helpers may lead to someone accidentally deleting the helper files when deleting the dir to regenerate the proto (e.g. a proto message was removed and must now be deleted manually from the gen package), however, it might be just okay, and since helpers would be easier to find from the same package+version we have to maintain fewer packages at the end.

I don't think there is a problem with it. 👍 Of course, the same helpers would be required in every version package (acl/v1alpha1, acl/v1beta1, acl/v1, ...) to use its versioned struct types respectively.

@zepatrik
Copy link
Member Author

Thanks, that makes sense. I also like the idea of having the helpers tied to the version, as that is required anyways. Checking that they will not be deleted should be possible to do during PR reviews, so I don't think it will be an issue.

@zepatrik zepatrik added the corp/m8 Up for M8 at Ory Corp. label May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
corp/m8 Up for M8 at Ory Corp.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants