Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Adjust workload process metadata to changes in the spec. #137
Conversation
ericsnowcurrently
added some commits
Jun 9, 2015
kat-co
reviewed
Jun 10, 2015
| -func (c processTypeOptionChecker) Coerce(v interface{}, path []string) (interface{}, error) { | ||
| - return fmt.Sprintf("%v", v), nil | ||
| +func (c forcedStringChecker) Coerce(v interface{}, path []string) (interface{}, error) { |
kat-co
reviewed
Jun 10, 2015
| - return nil, fmt.Errorf("%s: expected int got %q", strings.Join(path[1:], ""), parts[0]) | ||
| + portA := 0 | ||
| + external := parts[0] | ||
| + if !strings.HasPrefix(external, "<") || !strings.HasSuffix(external, ">") { |
kat-co
Jun 10, 2015
Contributor
It's not immediately evident what this chunk of code is trying to accomplish. Please add a few pointer comments.
ericsnowcurrently
added some commits
Jun 10, 2015
kat-co
reviewed
Jun 17, 2015
| + | ||
| +// ProcessFieldValue describes a requested change to a Process. | ||
| +type ProcessFieldValue struct { | ||
| + Field string |
kat-co
reviewed
Jun 17, 2015
| + | ||
| +// Extend updates the Process with the provided value. If the | ||
| +// identified field is already set then Extend fails. | ||
| +func (p *Process) Extend(value ProcessFieldValue) error { |
kat-co
Jun 17, 2015
Contributor
This looks like the inverse of "Override(...)". Maybe extract the logic out into a private method, add a boolean (example name failIfExists) parameter, and then change your checks to be:
if p.Description != "" && failIfExists {
return fmt.Errorf(`"description" already set`) // Tweak the error message to fit the failIfExists case
ericsnowcurrently
Jun 18, 2015
Contributor
As a whole there are subtle differences which mean it's not quite the inverse. I actually started off trying to do what you are talking about and it ended up messier and harder to follow. This way is clear and explicit. If it turns out to be a problem (which I doubt) then we can refactor these pieces later.
kat-co
reviewed
Jun 17, 2015
| + // InternalMount is the path on the process. | ||
| + InternalMount string | ||
| + // Mode is the "ro" OR "rw" | ||
| + Mode string |
ericsnowcurrently
Jun 18, 2015
Contributor
I don't think so. Since Go doesn't check the values, a typedef doesn't help much, particularly since it's a string. In fact it kind of makes the code messier and harder to follow. We have all the validation we need for the two values so I think it's fine as-is. Given that we rely pretty heavily on serialization and parsing for the metadata, let's just keep this as strings.
|
I believe we've addressed all your comments. If you feel the responses are satisfactory then are we good to go? |
|
LGTM |
added a commit
that referenced
this pull request
Jun 18, 2015
ericsnowcurrently commentedJun 10, 2015
No description provided.