Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add a supported series helper function #159
Conversation
axw
reviewed
Sep 30, 2015
| +// If the specified series is empty, the any default series defined by the | ||
| +// charm is used. Any specified series is validated against those supported | ||
| +// by the charm. | ||
| +func SeriesToUse(series string, supportedSeries []string) (string, error) { |
axw
Sep 30, 2015
Member
Rename the first parameter to "requestedSeries"? I think that would make its purpose a bit more obvious.
axw
reviewed
Sep 30, 2015
| + | ||
| +// SeriesToUse takes a specified series and a list of series supported by a | ||
| +// charm and returns the series which is relevant. | ||
| +// If the specified series is empty, the any default series defined by the |
axw
Sep 30, 2015
Member
s/the any/then any/
Is it really any? you're selecting the first one, and in the code you say "charm default". Is the charm default the first value in the list? If so, you should not say "any" here.
wallyworld
Sep 30, 2015
Owner
"any" needs to be read as "any default"
If the charm does not specify series, the default series is ""
The default is the first series specified, if any are specified.
axw
Sep 30, 2015
Member
If you're saying it's written as intended, then I do not understand it. I would write it as:
If the requested series is empty, the charm's default series (first item in the supported series list) is used.
Is that correct?
axw
reviewed
Sep 30, 2015
| +// SeriesToUse takes a specified series and a list of series supported by a | ||
| +// charm and returns the series which is relevant. | ||
| +// If the specified series is empty, the any default series defined by the | ||
| +// charm is used. Any specified series is validated against those supported |
wallyworld
Sep 30, 2015
Owner
Reworded:
// If the requested series is empty, then any default series defined by the
// charm is used, otherwise the requested series is validated against those
// supported by the charm.
axw
reviewed
Sep 30, 2015
| + if series == "" { | ||
| + return supportedSeries[0], nil | ||
| + } | ||
| + // Ensure series is supported. |
|
A few doc issues, otherwise LGTM. I don't like the name, but it'll do. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-charm |
added a commit
that referenced
this pull request
Sep 30, 2015
wallyworld commentedSep 30, 2015
Add a new SeriesToUse() helper for use by core and charmrepo.