generate: Move Generator.spec to Generator.Config #266
Conversation
|
In my opinion, I hope Generator could be a general library in the future. From our side, it can help us keep code of subcommand |
I agree, and I think this change will help us provide a better API. To help evaluate the change, I'll work up a follow-up PR that builds on this to transition some internal consumers, and we can decide if the transitioned code is more or less concise. Of course, there may be some benefit in buffering downstream users from runtime-caller Go type changes. I expect those to drop off as runtime-caller stabilizes, but "drop off" isn't "stop". Whether that isolation is worth the cost of writing and maintaining per-property wrappers is a maintainer call. |
|
On Mon, Nov 07, 2016 at 07:33:35PM -0800, W. Trevor King wrote:
Pushed with #267. |
|
It is already a general library right? @Mashimiao |
|
Conflicts detected, other wise LGTM |
|
There is an error, please refer to travis-ci log |
|
On Tue, Nov 22, 2016 at 05:12:11PM -0800, Ma Shimiao wrote:
|
|
need another rebase |
|
LGTM |
|
@wking need another rebase |
wking
force-pushed
the
public-config
branch
2 times, most recently
from
5ce5e35 to
986f514
Compare
|
On Mon, Feb 13, 2017 at 07:21:04PM -0800, Ma Shimiao wrote:
@wking need another rebase
|
|
LGTM |
|
Need rebase. |
This one is a pain to rebase ;). It's milestoned 0.6.0 now, so it's probably better to just wait and rebaae after 0.5.0 is cut and we have a better chance of collecting two LGTMs before it goes stale again ;). |
|
@wking It's already 0.5, please update. |
|
This is going to conflict with #194, so I'll wait and rebase this one after that settles. |
This makes the attribute public, since quite a few config manipulations are easier using Go's types than they are via getter/setter/mutator methods. This also means that we can drop methods that are more awkward than direct access (although we'll want to keep methods that are more convenient than direct access). I haven't done any method-dropping in this commit though, although I have deprecated a few of the more redundant methods. I'd called for this back when we started adding these methods [1], and Mrunal was sounding more positive about the public-attribute approach a few weeks ago [2]. I've also renamed this from "spec" to "config", because it is a config. I'm not sure why runtime-spec decided to call the main config.go type 'Spec' [3], but I don't think we should repeat that idiosyncrasy. [1]: opencontainers#137 (comment) [2]: opencontainers#253 (comment) [3]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc2/specs-go/config.go#L6 Signed-off-by: W. Trevor King <wking@tremily.us>
|
LGTM |
This makes the attribute public, since quite a few config manipulations are easier using Go's types than they are via getter/setter/mutator methods. This also means that we can drop methods that are more awkward than direct access (although we'll want to keep methods that are more convenient than direct access). I haven't done any method-dropping in this commit though, aside from the getter/setter for the config itself. I'd called for this back when we started adding these methods, and @mrunalp was sounding more positive about the public-attribute approach a few weeks ago.
I've also renamed this from “spec” to “config”, because it is a config. I'm not sure why runtime-spec decided to call the main
config.gotypeSpec, but I don't think we should repeat that idiosyncrasy. This breaks our current library API in a few places (e.g.NewFromSpec→NewFromConfig), but the migration shouldn't be that bad and I think it's worth it to make life easier for future consumers.