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 rid constructors #134

Merged
merged 2 commits into from
Dec 19, 2018
Merged

Add rid constructors #134

merged 2 commits into from
Dec 19, 2018

Conversation

k-simons
Copy link
Contributor

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

  • Add rid constructors for when you know the pieces of the identifier
  • Add additional tests (caught no incorrect functionality 👍 )

This change is Reviewable

Copy link
Contributor

@bmoylan bmoylan left a comment

Choose a reason for hiding this comment

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

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


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

}

func Of(service, instance, resourceType, locator string) (ResourceIdentifier, error) {

more idiomatic go would be New(...)


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

func (rid ResourceIdentifier) String() string {
	return strings.Join([]string{RidClass, rid.Service, rid.Instance, rid.Type, rid.Locator}, Separator)

I did the appends myself for performance reasons; strings.Join is quite slow.


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

// ParseRID parses a string into a 4-part resource identifier.
func ParseRID(s string) (ResourceIdentifier, error) {
	segments := strings.SplitN(s, ".", 5)

can use the Separator constant here


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

func ParseRID(s string) (ResourceIdentifier, error) {
	segments := strings.SplitN(s, ".", 5)
	if len(segments) != 5 || segments[0] != "ri" {

can use the RidClass constant here

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: all files reviewed, 4 unresolved discussions (waiting on @bmoylan)


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

Previously, bmoylan (Brad Moylan) wrote…

more idiomatic go would be New(...)

Done.


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

Previously, bmoylan (Brad Moylan) wrote…

I did the appends myself for performance reasons; strings.Join is quite slow.

Done.


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

Previously, bmoylan (Brad Moylan) wrote…

can use the Separator constant here

Done.


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

Previously, bmoylan (Brad Moylan) wrote…

can use the RidClass constant here

Done.

Copy link
Contributor

@bmoylan bmoylan left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @bmoylan)

Copy link
Contributor

@bmoylan bmoylan 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 r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@bmoylan bmoylan merged commit 31253b8 into palantir:master Dec 19, 2018
@nmiyake
Copy link
Contributor

nmiyake commented Dec 19, 2018


rid/resource_identifier.go, line 38 at r2 (raw file):

const (
	RidClass  = "ri"

Is there a reason these constants need to be exported? Seems like they should be private.

Also, if exported, should be RIDClass.

@nmiyake
Copy link
Contributor

nmiyake commented Dec 19, 2018


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

)

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

Where is MustNew intended to be used? I would prefer not to include this as part of the actual API (if there's a client/tests that want to do this, they can easily implement this on their end).

@nmiyake
Copy link
Contributor

nmiyake commented Dec 19, 2018

I see that this merged, but have some feedback that I think we should work through/address before cutting a release.

@k-simons
Copy link
Contributor Author

@nmiyake will open a follow up to discuss

@k-simons
Copy link
Contributor Author

#135

@k-simons k-simons deleted the ksimons/ridHelpers branch December 19, 2018 18:06
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.

3 participants