Skip to content

abstract the string slice struct to stringutils package#15913

Merged
runcom merged 1 commit intomoby:masterfrom
mountkin:abstract
Sep 1, 2015
Merged

abstract the string slice struct to stringutils package#15913
runcom merged 1 commit intomoby:masterfrom
mountkin:abstract

Conversation

@mountkin
Copy link
Contributor

The logic of runconfig.Entrypoint and runconfig.Command are almost the same.
While I'm working on #15780 I find that I also need the string or slice parsing utils.
So I think abstracting the parsing code to a business independent package may be useful for other similar functions.

@calavera
Copy link
Contributor

Very nice. LGTM

Signed-off-by: Shijiang Wei <mountkin@gmail.com>
@aaronlehmann
Copy link

I like the idea of combining these types. But I'm not sure I see the value of having an abstraction around []string to replace them. I'd think the original idea of having the specific types Entrypoint and Command was to abstract away the fact that they are implemented as []string, and let them evolve in the future (add new fields, etc). But if we change the type to StrSlice, then they are defined as being []string, but in a roundabout way.

What about just using []string as the type, and having the NewStrSlice convenience function return []string directly? Then we wouldn't need the extra wrappers for JSON marshalling/unmarshalling, Len, Slice, etc.

@mountkin
Copy link
Contributor Author

@aaronlehmann we can't use []string directly because the cmd or entrypoint may be a string or a slice of strings. If we use []string a BC break will be introduced.

If in the future we need to add more fields to the structure, we can just create a new struct in runconfig and embed the StrSlice struct.

@calavera
Copy link
Contributor

@aaronlehmann, @mountkin is right. We need this struct to support arguments that can be a string and a slice of strings at the same time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, only need to define the type here once since both args are the same type.

@cpuguy83
Copy link
Member

cpuguy83 commented Sep 1, 2015

LGTM

@runcom
Copy link
Member

runcom commented Sep 1, 2015

This may also be useful if we should continue supporting old []string which accept a string as well (see #14338)

LGTM

runcom added a commit that referenced this pull request Sep 1, 2015
abstract the string slice struct to stringutils package
@runcom runcom merged commit 4bb2449 into moby:master Sep 1, 2015
@mountkin mountkin deleted the abstract branch September 2, 2015 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants