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
Refactor ParseTag #5
Conversation
davecheney
commented
Jun 16, 2014
- refactor ParseTag, removing the hint. The hint was only used by code when it expected a specific tag type.
- introduce type specific Tag parsers, these replace all the uses of ParseTag with hint, and return a specific Tag implementation, not an interface.
- Add unit tests for new methods
* refactor ParseTag, removing the hint. The hint was only used by code when it expected a specific tag type. * introduce type specific Tag parsers, these replace all the uses of ParseTag with hint and return a specific Tag implementation, not an interface, so the Tag's implemenations values are available without calling a helper method * Add unit tests for new methods
* refactor ParseTag, removing the hint. The hint was only used by code when it expected a specific tag type. * introduce type specific Tag parsers, these replace all the uses of ParseTag with hint and return a specific Tag implementation, not an interface, so the Tag's implemenations values are available without calling a helper method * Add unit tests for new methods
…/names into 106-parsetag-refactor
tag: "", | ||
err: names.InvalidTagError("", ""), | ||
}, { | ||
tag: "machine-0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a gofmt bug, http://code.google.com/p/go/issues/detail?id=8215
Done. |
PTAL. I tweaked the example, but that was all I changed. |
If the Parse functions are going to return specific tag types, shouldn't the New* functions do likewise? |
They can do that, I can address that in a followup branch. On Mon, Jun 16, 2014 at 5:31 PM, rogpeppe notifications@github.com wrote:
|
LGTM |