Skip to content

Refactor utils/utils, fixes #11923#11992

Merged
estesp merged 1 commit intomoby:masterfrom
runcom:11923-refactor-utils-utils
Apr 14, 2015
Merged

Refactor utils/utils, fixes #11923#11992
estesp merged 1 commit intomoby:masterfrom
runcom:11923-refactor-utils-utils

Conversation

@runcom
Copy link
Copy Markdown
Member

@runcom runcom commented Apr 1, 2015

Pretty big changes here, sorry for this. (@jfrazelle I've read you had brain explosion while cherry-picking commits for release after one of my "refactors", again, sorry 😅)
The whole point is to get rid of utils/utils unless strictly needed for docker's thingies.
Only functions left in utils/utils are, as said, functions needed in the project that either cannot be in pkgs (because docker related) or cannot be moved to specific part of the project (because used in many places).
Did it for fun and because I don't like utils pattern.. Feel free to close this if it's totally unnecessary.

utils/utils function Renamed? Now in
Trunc Truncate pkg/stringutils
StringsContainsNocase InSlice pkg/stringutils
Download NO pkg/httputils
CopyFile NO pkg/fileutils
ValidateID NO image.go
WriteCounter* NO pkg/ioutils/writers.go
GetTotalUsedFds NO pkg/fileutils
CopyEscapable copyEscapable attach.go
HashData NO pkg/ioutils/reader.go
ShellQuoteArguments NO pkg/stringutils
ReadSymlinkedDirectory NO pkg/fileutils
KeyValuePair (struct) NO runconfig
NewHTTPRequestError NO pkg/httputils
StatusError (struct) NO api/client

Fixes #11923.

Signed-off-by: Antonio Murdaca [email protected]

@icecrime
Copy link
Copy Markdown
Contributor

icecrime commented Apr 1, 2015

Holy 💩, nice work :-)

@runcom
Copy link
Copy Markdown
Member Author

runcom commented Apr 1, 2015

I'll fix test asap!

@icecrime
Copy link
Copy Markdown
Contributor

icecrime commented Apr 1, 2015

I'm not sure about StringsContainsNocase being renamed to InSlice: I believe the "case insensitive" aspect of the function was important enough to appear in the function name.

@runcom
Copy link
Copy Markdown
Member Author

runcom commented Apr 1, 2015

@icecrime same thought, I was going to add a bool wheater to compare sensitive or insensitive to InSlice, what do you think? Otherwise I'll just rename it back

@icecrime
Copy link
Copy Markdown
Contributor

icecrime commented Apr 1, 2015

Sorry, I just deleted a stupid comment of mine (string.Contains is something entirely different).

@runcom
Copy link
Copy Markdown
Member Author

runcom commented Apr 1, 2015

alright, I was using InSlice as other languages has this type of function on arrays/hash, so I'm more into adding a bool, or if needed leave it insensitive as many programming languages do and that's all

@runcom runcom force-pushed the 11923-refactor-utils-utils branch 2 times, most recently from a12b117 to 4611a93 Compare April 1, 2015 20:48
@runcom
Copy link
Copy Markdown
Member Author

runcom commented Apr 1, 2015

tests fixed @icecrime

@jessfraz
Copy link
Copy Markdown
Contributor

jessfraz commented Apr 1, 2015

ooo cool table

@jessfraz
Copy link
Copy Markdown
Contributor

jessfraz commented Apr 1, 2015

this is legit!

@runcom
Copy link
Copy Markdown
Member Author

runcom commented Apr 7, 2015

gonna rebase this if it's still ok! many many conflicts lol

@jessfraz
Copy link
Copy Markdown
Contributor

jessfraz commented Apr 9, 2015

Yes we def want

@runcom
Copy link
Copy Markdown
Member Author

runcom commented Apr 9, 2015

This is gonna be a mess 😩 I'll do it asap!

@runcom runcom force-pushed the 11923-refactor-utils-utils branch from 4611a93 to 39084d6 Compare April 11, 2015 14:47
@runcom
Copy link
Copy Markdown
Member Author

runcom commented Apr 11, 2015

@jfrazelle rebased

thaJeztah
thaJeztah reviewed Apr 11, 2015
View reviewed changes
Comment thread image/image.go
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Apr 11, 2015

Choose a reason for hiding this comment

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

Should this be moved to pkg/stringid in stead? That's also used to generate IDs

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Apr 11, 2015

Choose a reason for hiding this comment

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

^^ just thinking out loud; I'm not a maintainer

Copy link
Copy Markdown
Member Author

@runcom runcom Apr 11, 2015

Choose a reason for hiding this comment

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

@thaJeztah ValidateID is a docker specific routine to check whether the image id is valid or not and cannot be included in pkg/stringid because whole pkgs purpose is about reusable code not tied to docker internals. Plus it's already namespaced here so image.ValidateID is just about validating image ids.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Apr 11, 2015

Choose a reason for hiding this comment

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

hm, yes. stringid.TruncateID() is quite docker-specific though, because of the arbitrary length. Think that put me on the wrong foot.

Copy link
Copy Markdown
Member Author

@runcom runcom Apr 11, 2015

Choose a reason for hiding this comment

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

mmm I don't know, the stringid/truncindex are tied but can be useful outside docker too I suppose.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Apr 11, 2015

Choose a reason for hiding this comment

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

yeah, I just thought I'd mention it. I was thinking of creating an (non related) issue because I noticed some parts of the code used stringid.TruncateID() and some parts used a literal id[:12], then I stumbled upon this PR 😄

@runcom runcom force-pushed the 11923-refactor-utils-utils branch from 39084d6 to b295d30 Compare April 13, 2015 23:00
Refactor utils/utils, fixes moby#11923
c30a55f 
Signed-off-by: Antonio Murdaca <[email protected]>
@runcom runcom force-pushed the 11923-refactor-utils-utils branch from b295d30 to c30a55f Compare April 13, 2015 23:37
@runcom
Copy link
Copy Markdown
Member Author

runcom commented Apr 14, 2015

ping @LK4D4 rebased and moved StatusError to api/client.go

LK4D4
LK4D4 reviewed Apr 14, 2015
View reviewed changes
Comment thread api/client/build.go
Copy link
Copy Markdown
Contributor

@LK4D4 LK4D4 Apr 14, 2015

Choose a reason for hiding this comment

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

Btw makes sense to make it value, not pointer. It isn't big deal for client though, just matter of Go-style. You can create issue for newbie contributors about it.

Copy link
Copy Markdown
Member Author

@runcom runcom Apr 14, 2015

Choose a reason for hiding this comment

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

👍

@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Apr 14, 2015

LGTM

1 similar comment
@estesp
Copy link
Copy Markdown
Contributor

estesp commented Apr 14, 2015

LGTM

estesp added a commit that referenced this pull request Apr 14, 2015
@estesp
Merge pull request #11992 from runcom/11923-refactor-utils-utils
fe53c27 
Refactor utils/utils, fixes #11923
@estesp estesp merged commit fe53c27 into moby:master Apr 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor utils/utils

7 participants

@runcom @icecrime @jessfraz @LK4D4 @estesp @thaJeztah @GordonTheTurtle