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

Don't export rid constants #135

Merged
merged 1 commit into from
Dec 19, 2018
Merged

Conversation

k-simons
Copy link
Contributor

@k-simons k-simons commented Dec 19, 2018

  • Don't export rid constants

This change is Reviewable

Copy link
Contributor Author

@k-simons k-simons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion


rid/resource_identifier.go, line 42 at r1 (raw file):

)

func MustNew(service, instance, resourceType, locator string) ResourceIdentifier {

MustNew will be used by any client that has a static rid that will never change and will not throw an error. This is a common enough use case that I believe the rids package should provide a helper methods. Yes it could be implemented by every single client, but its the same reason https://github.com/palantir/pkg/blob/master/metrics/tag.go#L121 exists

@k-simons k-simons mentioned this pull request Dec 19, 2018
Copy link
Contributor

@nmiyake nmiyake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @k-simons)


rid/resource_identifier.go, line 42 at r1 (raw file):

Previously, k-simons wrote…

MustNew will be used by any client that has a static rid that will never change and will not throw an error. This is a common enough use case that I believe the rids package should provide a helper methods. Yes it could be implemented by every single client, but its the same reason https://github.com/palantir/pkg/blob/master/metrics/tag.go#L121 exists

OK yup if instantiation of static RIDs is a common use case then that makes sense and I agree -- thanks for the clarification!

Copy link
Contributor

@nmiyake nmiyake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @k-simons)

Copy link
Contributor

@nmiyake nmiyake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @k-simons from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@nmiyake nmiyake merged commit f026ea9 into palantir:master Dec 19, 2018
@k-simons k-simons deleted the ksimons/ridFollowUps branch December 19, 2018 18: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.

2 participants