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 bearertoken, datetime, rid, safelong, and uuid packages #132

Merged
merged 2 commits into from
Dec 19, 2018

Conversation

bmoylan
Copy link
Contributor

@bmoylan bmoylan commented Dec 19, 2018

This change is Reviewable

@bmoylan bmoylan requested a review from nmiyake December 19, 2018 06:15
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.

Question about whether deprecated type definition needs to be included and feedback on license headers for copied code

Reviewed 16 of 16 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bmoylan)


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

// Rid is deprecated: use ResourceIdentifier.
type Rid string

Do we need do declare this deprecated type here?


uuid/internal/uuid/README.md, line 1 at r1 (raw file):

**conjure-go note**: This package was adapted from https://github.com/google/uuid/tree/v1.1.0

Should remove conjure-go from this note


uuid/internal/uuid/util.go, line 1 at r1 (raw file):

// Copyright (c) 2018 Palantir Technologies. All rights reserved.

We should update config to exclude the internal packages from license generation and remove the Palantir portion

Copy link
Contributor Author

@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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nmiyake)


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

Previously, nmiyake (Nick Miyake) wrote…

Do we need do declare this deprecated type here?

good catch, removed


uuid/internal/uuid/README.md, line 1 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Should remove conjure-go from this note

done


uuid/internal/uuid/util.go, line 1 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

We should update config to exclude the internal packages from license generation and remove the Palantir portion

yup, had the exclude in the other repo but forgot to port it here. done.

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.

:lgtm: -- thanks!

Reviewed 8 of 8 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@bmoylan bmoylan merged commit d0bd127 into master Dec 19, 2018
@bmoylan bmoylan deleted the bm/conjuretype branch December 19, 2018 06:38
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