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

juju/names refactor part 2 #3

Merged
merged 5 commits into from Jun 12, 2014
Merged

juju/names refactor part 2 #3

merged 5 commits into from Jun 12, 2014

Conversation

davecheney
Copy link
Contributor

Introduce tag.Id() and tag.Kind() methods.

Prepare to alter the signature for ParseTag to return (Tag, error)

finish adding Id()
Prepare to alter the signature for ParseTag to return (Tag, error)
Prepare to alter the signature for ParseTag to return (Tag, error)
@rogpeppe
Copy link
Contributor

In passing, why not just "type EnvironTag string" (and the others) ?

@davecheney
Copy link
Contributor Author

On Wed, Jun 11, 2014 at 6:10 PM, rogpeppe notifications@github.com wrote:

In passing, why not just "type EnvironTag string" (and the others) ?

Two reasons, one major, one minor.

The major one was when I started I wasn't sure what kind of data each type
would have to model so making them all a struct to insure against adding
more data later. It has turned out that this wasn't needed.

The minor concern was to prevent people converting back to a string, by
making name (or id, or uuid, or key, or whatever) a private field, people
cannot convert a Tag implementation into a string out of frustration. I'm
open to arguments on both sides.


Reply to this email directly or view it on GitHub
#3 (comment).

@howbazaar
Copy link
Contributor

I do have to admit to preferring the struct approach as it actually names the string, like uuid for environment.

I've also been thinking in passing that a RemoteUserTag may well have two parts: username and identity provider (username@identity-provider)

@@ -148,7 +148,7 @@ func (*tagSuite) TestParseTag(c *gc.C) {
kind, id, err := names.ParseTag(test.tag, test.expectKind)
Copy link
Contributor

Choose a reason for hiding this comment

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

since the result of the ParseTag function has changed I think
tag, id, err
would be better variable names.
Then kind.Kind() becomes tag.Kind() below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@howbazaar
Copy link
Contributor

LGTM, we can refactor the tests later.

@davecheney
Copy link
Contributor Author

Thanks. The very next branch refactors the test. This is just an interrum
step to make sure I the id value from ParseTag and Tag.Id() did not diverge.

On Thu, Jun 12, 2014 at 2:51 PM, Tim Penhey notifications@github.com
wrote:

LGTM, we can refactor the tests later.


Reply to this email directly or view it on GitHub
#3 (comment).

davecheney added a commit that referenced this pull request Jun 12, 2014
@davecheney davecheney merged commit bbf47e1 into juju:master Jun 12, 2014
@davecheney davecheney deleted the 104-introduce-tags-type-ii branch June 12, 2014 05:41
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.

None yet

3 participants