Skip to content

fix golint warnings/errors on package api/types/#14897

Merged
calavera merged 1 commit intomoby:masterfrom
WeiZhang555:golint-api-types
Aug 7, 2015
Merged

fix golint warnings/errors on package api/types/#14897
calavera merged 1 commit intomoby:masterfrom
WeiZhang555:golint-api-types

Conversation

@WeiZhang555
Copy link
Contributor

@WeiZhang555 WeiZhang555 commented Jul 23, 2015

Part of #14756
Let me know if anyone has suggestions or questions, thank you!

Signed-off-by: Zhang Wei zhangwei555@huawei.com

@WeiZhang555
Copy link
Contributor Author

WeiZhang555 commented Jul 23, 2015

Seems there are some failed test cases, I'll check the code and re-open this PR after fixing all bugs.

@WeiZhang555 WeiZhang555 reopened this Jul 24, 2015
@WeiZhang555 WeiZhang555 force-pushed the golint-api-types branch 3 times, most recently from 909c677 to cf1bb5d Compare July 24, 2015 18:03
vdemeester
vdemeester reviewed Jul 24, 2015
View reviewed changes
api/types/types.go Outdated
Copy link
Member

@vdemeester vdemeester Jul 24, 2015

Choose a reason for hiding this comment

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

As it is serialized as JSON, I think it would be better to do something like :

ParentID string `json:"ParentId"`

This way, this limit the possible breakage of tools using the API.

vdemeester
vdemeester reviewed Jul 24, 2015
View reviewed changes
api/types/types.go Outdated
Copy link
Member

@vdemeester vdemeester Jul 24, 2015

Choose a reason for hiding this comment

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

Same as above, as it is serialized as JSON, I think it would be better to do something like :

HTTPProxy string `json:"HttpProxy"`
HTTPSProxy string `json:"HttpsProxy"`

@vdemeester
Copy link
Member

vdemeester commented Jul 25, 2015

@WeiZhang555 I think you should rebase to make the integration-cli test passes (I'm pretty sure a fix has been merged 😉 ).

@WeiZhang555
Copy link
Contributor Author

WeiZhang555 commented Jul 25, 2015

@vdemeester Updated, let's see if this commit works better. 😄

@thaJeztah thaJeztah changed the title fix goling warnings/errors on package api/types/ fix golint warnings/errors on package api/types/ Jul 25, 2015
@WeiZhang555
Copy link
Contributor Author

WeiZhang555 commented Jul 28, 2015

Rebased

@WeiZhang555
Copy link
Contributor Author

WeiZhang555 commented Jul 31, 2015

Rebased again.

@vdemeester
Copy link
Member

vdemeester commented Jul 31, 2015

@WeiZhang555 could you take care of my comments about the structure and the json:… stuff ? This will make those changes less error prone — you shouldn't have to modify the tests that uses the API (like inspectField(…)) because this means it breaks a little bit the API and thus it could break external tools that consume the API (like docker/compose, kubernetes, etc..). 😊

@WeiZhang555
Copy link
Contributor Author

WeiZhang555 commented Jul 31, 2015

@vdemeester Yeah, I'm so sorry, I must have missed that, I would add json part and have a double check.

@WeiZhang555
Copy link
Contributor Author

WeiZhang555 commented Jul 31, 2015

@vdemeester Updated, sorry for my careless, I have added the json stuff. You are right, breaking the json API format is bad and we should try hard to avoid this kind of modification unless we have to.
And the code changing with json stuff is more reasonable and elegant now, thank you! 😄

@vdemeester
Copy link
Member

vdemeester commented Jul 31, 2015

@WeiZhang555 don't be sorry 😝, this is part a the learning and the contributing process 😊 😉.

@tiborvass
Copy link
Contributor

tiborvass commented Aug 3, 2015

@WeiZhang555 really sorry but needs another rebase :(

@WeiZhang555
Copy link
Contributor Author

WeiZhang555 commented Aug 4, 2015

@tiborvass Rebase done :)

@WeiZhang555
Copy link
Contributor Author

WeiZhang555 commented Aug 5, 2015

Rebased

@lowenna
Copy link
Member

lowenna commented Aug 5, 2015

Needs another rebase to pick up the compile error on the Windows daemon build.

@icecrime icecrime mentioned this pull request Aug 5, 2015
Closed
@WeiZhang555
Copy link
Contributor Author

WeiZhang555 commented Aug 6, 2015

All green now. Ping @vdemeester @tiborvass @icecrime @jhowardmsft

@lowenna
Copy link
Member

lowenna commented Aug 6, 2015

LGTM

vdemeester
vdemeester reviewed Aug 6, 2015
View reviewed changes
api/types/types.go Outdated
Copy link
Member

@vdemeester vdemeester Aug 6, 2015

Choose a reason for hiding this comment

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

This one probably need a json:… too 😅

Copy link
Contributor

@abronan abronan Aug 6, 2015

Choose a reason for hiding this comment

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

Nice catch 👍

@WeiZhang555
fix golint warnings/errors on package api/types/
3d6617f 
Signed-off-by: Zhang Wei <zhangwei555@huawei.com>
@WeiZhang555
Copy link
Contributor Author

WeiZhang555 commented Aug 7, 2015

@vdemeester Updated and rebased, thank you! 😄

@calavera
Copy link
Contributor

calavera commented Aug 7, 2015

LGTM. Merging!

calavera added a commit that referenced this pull request Aug 7, 2015
@calavera
Merge pull request #14897 from WeiZhang555/golint-api-types
0a0e970 
fix golint warnings/errors on package api/types/
@calavera calavera merged commit 0a0e970 into moby:master Aug 7, 2015
@WeiZhang555 WeiZhang555 deleted the golint-api-types branch August 8, 2015 06:12
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.

8 participants

@WeiZhang555 @vdemeester @tiborvass @lowenna @calavera @runcom @abronan @GordonTheTurtle