Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5209)

Issue 12473043: names: add ParseTag

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 9 months ago by rog
Modified:
8 years, 9 months ago
Reviewers:
dimitern, mp+178619, natefinch, jameinel
Visibility:
Public.

Description

names: add ParseTag This simplifies State.entity (shortly to be State.Entity) and the names package itself. https://code.launchpad.net/~rogpeppe/juju-core/362-names-parse-tag/+merge/178619 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : names: add ParseTag #

Patch Set 3 : names: add ParseTag #

Total comments: 21

Patch Set 4 : names: add ParseTag #

Patch Set 5 : names: add ParseTag #

Patch Set 6 : names: add ParseTag #

Patch Set 7 : names: add ParseTag #

Total comments: 4

Patch Set 8 : names: add ParseTag #

Unified diffs Side-by-side diffs Delta from patch set Stats (+214 lines, -122 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M names/environ.go View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
M names/machine.go View 1 2 3 4 5 2 chunks +5 lines, -14 lines 0 comments Download
M names/machine_test.go View 1 2 1 chunk +0 lines, -26 lines 0 comments Download
M names/tag.go View 1 2 3 4 5 6 7 2 chunks +41 lines, -0 lines 0 comments Download
M names/tag_test.go View 1 2 3 4 3 chunks +113 lines, -3 lines 0 comments Download
M names/unit.go View 1 2 3 4 5 1 chunk +9 lines, -18 lines 0 comments Download
M names/unit_test.go View 1 2 1 chunk +0 lines, -19 lines 0 comments Download
A names/user.go View 1 2 1 chunk +12 lines, -0 lines 0 comments Download
M state/api/deployer/unit.go View 1 chunk +1 line, -1 line 0 comments Download
M state/apiserver/client/client_test.go View 1 1 chunk +1 line, -1 line 0 comments Download
M state/apiserver/deployer/deployer.go View 2 chunks +2 lines, -2 lines 0 comments Download
M state/apiserver/login_test.go View 1 1 chunk +1 line, -1 line 0 comments Download
M state/apiserver/machine/agent.go View 1 chunk +1 line, -1 line 0 comments Download
M state/state.go View 1 2 3 4 5 6 7 2 chunks +9 lines, -31 lines 0 comments Download
M state/state_test.go View 1 2 3 4 5 6 7 1 chunk +9 lines, -5 lines 0 comments Download

Messages

Total messages: 10
rog
Please take a look.
8 years, 9 months ago (2013-08-06 09:21:08 UTC) #1
dimitern
Looks really good, just a few suggestions. https://codereview.appspot.com/12473043/diff/6001/names/machine.go File names/machine.go (right): https://codereview.appspot.com/12473043/diff/6001/names/machine.go#newcode1 names/machine.go:1: // Copyright ...
8 years, 9 months ago (2013-08-06 09:35:39 UTC) #2
rog
https://codereview.appspot.com/12473043/diff/6001/names/machine.go File names/machine.go (right): https://codereview.appspot.com/12473043/diff/6001/names/machine.go#newcode31 names/machine.go:31: func machineFromTagName(name string) string { On 2013/08/06 09:35:40, dimitern ...
8 years, 9 months ago (2013-08-06 09:50:08 UTC) #3
dimitern
LGTM, but I'd like to see the "test %d." changed to "test %d:" please. https://codereview.appspot.com/12473043/diff/6001/names/machine.go ...
8 years, 9 months ago (2013-08-06 10:00:28 UTC) #4
rog
Please take a look. https://codereview.appspot.com/12473043/diff/6001/names/machine.go File names/machine.go (right): https://codereview.appspot.com/12473043/diff/6001/names/machine.go#newcode31 names/machine.go:31: func machineFromTagName(name string) string { ...
8 years, 9 months ago (2013-08-06 10:04:20 UTC) #5
rog
Please take a look.
8 years, 9 months ago (2013-08-06 10:19:11 UTC) #6
rog
Please take a look.
8 years, 9 months ago (2013-08-07 11:30:27 UTC) #7
natefinch
LGTM with just a couple very minor things. https://codereview.appspot.com/12473043/diff/30001/names/tag.go File names/tag.go (right): https://codereview.appspot.com/12473043/diff/30001/names/tag.go#newcode70 names/tag.go:70: return ...
8 years, 9 months ago (2013-08-07 12:30:17 UTC) #8
rog
Please take a look. https://codereview.appspot.com/12473043/diff/30001/names/tag.go File names/tag.go (right): https://codereview.appspot.com/12473043/diff/30001/names/tag.go#newcode70 names/tag.go:70: return "", "", fmt.Errorf("%q is ...
8 years, 9 months ago (2013-08-07 12:55:51 UTC) #9
jameinel
8 years, 9 months ago (2013-08-08 18:09:05 UTC) #10
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b