Skip to content

golint fixes for daemon/logger/*#14843

Merged
tiborvass merged 1 commit intomoby:masterfrom
MHBauer:demonlogger-lint
Jul 29, 2015
Merged

golint fixes for daemon/logger/*#14843
tiborvass merged 1 commit intomoby:masterfrom
MHBauer:demonlogger-lint

Conversation

@MHBauer
Copy link
Contributor

@MHBauer MHBauer commented Jul 22, 2015

golint advice changes
case statement resorted by alphabetical
package comments based on my understanding as a n00b
for #14756

vdemeester
vdemeester reviewed Jul 22, 2015
View reviewed changes
daemon/logger/fluentd/fluentd.go Outdated
Copy link
Member

@vdemeester vdemeester Jul 22, 2015

Choose a reason for hiding this comment

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

There is a /* too much 😛 here.

@icecrime icecrime mentioned this pull request Jul 22, 2015
Closed
@MHBauer MHBauer force-pushed the demonlogger-lint branch from b6766fb to a4c1c0d Compare July 22, 2015 17:27
@MHBauer
Copy link
Contributor Author

MHBauer commented Jul 22, 2015

comments fixed, and rebased

@runcom
Copy link
Member

runcom commented Jul 23, 2015

@MHBauer sorry this needs another rebase :(

@MHBauer MHBauer force-pushed the demonlogger-lint branch 4 times, most recently from 3057f6a to 35e6944 Compare July 23, 2015 21:33
@MHBauer
Copy link
Contributor Author

MHBauer commented Jul 23, 2015

It would be so good, if I stopped messing up rebases.

@MHBauer
Copy link
Contributor Author

MHBauer commented Jul 23, 2015

is there something I need to do here? Or just wait and trigger the build again?

@MHBauer MHBauer force-pushed the demonlogger-lint branch 2 times, most recently from a4ae777 to 1757d17 Compare July 24, 2015 18:29
vdemeester
vdemeester reviewed Jul 24, 2015
View reviewed changes
daemon/logger/factory.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.

Not sure if there's a rule in the docker source code but I think it would be better to have this comment on multiple lines (with a max of ~80 char per line). but that is a tiny tiny nit 😊

Copy link
Contributor Author

@MHBauer MHBauer Jul 24, 2015

Choose a reason for hiding this comment

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

my bad, forgot to hit format.

@MHBauer MHBauer force-pushed the demonlogger-lint branch 2 times, most recently from 3981db6 to 645bd4b Compare July 24, 2015 22:13
@vdemeester
Copy link
Member

vdemeester commented Jul 25, 2015

@MHBauer few things you can do :

  1. Rebase against master to get the fix for the failing test (Fix TestRunNonRootUserResolvName flackiness #14979).
  2. Make sure you add a period at the end of the function/method docs (it's a nit but 😝).
  3. You added a package doc for logger/fluentd but not for the others, could you add it for all implementation ? 😉

@MHBauer MHBauer force-pushed the demonlogger-lint branch 4 times, most recently from 1baceb4 to d15dee5 Compare July 28, 2015 17:49
RichardScothern
RichardScothern reviewed Jul 28, 2015
View reviewed changes
daemon/logger/journald/journald.go Outdated
Copy link
Contributor

@RichardScothern RichardScothern Jul 28, 2015

Choose a reason for hiding this comment

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

recieve -> receive

Copy link
Contributor Author

@MHBauer MHBauer Jul 28, 2015

Choose a reason for hiding this comment

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

well, this is embarrassing, so I've now figured out how to enable spell checking. Thanks

@RichardScothern
Copy link
Contributor

RichardScothern commented Jul 28, 2015

LGTM pending vdemeester's comments and one minor nit.

@MHBauer MHBauer force-pushed the demonlogger-lint branch 2 times, most recently from 399e5e4 to 51ce7d8 Compare July 28, 2015 20:23
@MHBauer
Copy link
Contributor Author

MHBauer commented Jul 28, 2015

rebased with everything addressed.

LK4D4
LK4D4 reviewed Jul 28, 2015
View reviewed changes
daemon/logger/factory.go Outdated
Copy link
Contributor

@LK4D4 LK4D4 Jul 28, 2015

Choose a reason for hiding this comment

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

I think it'll be more consistent with stdlib documentation to write just Creator builds..., and below too. Also it isn't method at all :)

Copy link
Contributor Author

@MHBauer MHBauer Jul 28, 2015

Choose a reason for hiding this comment

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

it's the java leaving my body.

@MHBauer MHBauer force-pushed the demonlogger-lint branch from 51ce7d8 to c10f21f Compare July 28, 2015 21:44
@gdevillele
Copy link
Contributor

gdevillele commented Jul 29, 2015

needs rebase

@MHBauer
golint fixes for daemon/logger/*
ccbe539 
 - downcase and privatize exported variables that were unused
 - make accurate an error message
 - added package comments
 - remove unused var ReadLogsNotSupported
 - enable linter
 - some spelling corrections

Signed-off-by: Morgan Bauer <mbauer@us.ibm.com>
@MHBauer MHBauer force-pushed the demonlogger-lint branch from c10f21f to ccbe539 Compare July 29, 2015 20:09
@MHBauer
Copy link
Contributor Author

MHBauer commented Jul 29, 2015

rebased

@gdevillele
Copy link
Contributor

gdevillele commented Jul 29, 2015

Thanks.
LGTM

@tiborvass
Copy link
Contributor

tiborvass commented Jul 29, 2015

LGTM

tiborvass added a commit that referenced this pull request Jul 29, 2015
@tiborvass
Merge pull request #14843 from MHBauer/demonlogger-lint
6a274e4 
golint fixes for daemon/logger/*
@tiborvass tiborvass merged commit 6a274e4 into moby:master Jul 29, 2015
@MHBauer MHBauer deleted the demonlogger-lint branch July 30, 2015 17:42
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.

9 participants

@MHBauer @runcom @vdemeester @RichardScothern @gdevillele @tiborvass @LK4D4 @jessfraz @GordonTheTurtle