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 Tag type for Action #6

Merged
merged 2 commits into from Jun 17, 2014
Merged

Add Tag type for Action #6

merged 2 commits into from Jun 17, 2014

Conversation

johnweldon
Copy link
Contributor

PTAL

// NewActionTag returns the tag for the action with the given name.
func NewActionTag(actionName string) Tag {
return ActionTag{name: actionName}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add a ParseActionTag method ? You don't need to, it's only needed if you don't want to type assert the result of ParseTag("someaction-...")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at this time; thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I did end up implementing this; thanks!

@johnweldon
Copy link
Contributor Author

Updated, PTAL

name string
}

func (t ActionTag) String() string { return t.Kind() + "-" + t.Id() }

Choose a reason for hiding this comment

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

It is nice if all public methods have a comment for godocs. Something like "String returns an ActionTag as a string type" or "human readable" or what ever the intent it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@wwitzel3
Copy link

LGTM I just had one optional / trivial comment.

Also as someone completely unfamiliar with this code I found this very readable and easy to understand, including the tests. Nice job.

@johnweldon
Copy link
Contributor Author

Thanks @wwitzel3 I'll make those changes and then merge.

johnweldon added a commit that referenced this pull request Jun 17, 2014
@johnweldon johnweldon merged commit 3b4956d into juju:master Jun 17, 2014
@johnweldon johnweldon deleted the actions branch June 17, 2014 22:09
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