Skip to content

golint on some pkg/* packages#14759

Merged
jessfraz merged 1 commit intomoby:masterfrom
vdemeester:pkg-golint
Jul 27, 2015
Merged

golint on some pkg/* packages#14759
jessfraz merged 1 commit intomoby:masterfrom
vdemeester:pkg-golint

Conversation

@vdemeester
Copy link
Member

I started to work on golint the pkg/* package 🐸.

  • pkg/useragent
  • pkg/units
  • pkg/ulimit
  • pkg/truncindex
  • pkg/timeoutconn
  • pkg/term
  • pkg/tarsum
  • pkg/tailfile
  • pkg/systemd
  • pkg/stringutils
  • pkg/stringid
  • pkg/streamformatter
  • pkg/sockets
  • pkg/signals
  • pkg/proxy
  • pkg/progressreader
  • pkg/pools
  • pkg/plugins
  • pkg/pidfile
  • pkg/parsers
  • pkg/parsers/filters
  • pkg/parsers/kernel
  • pkg/parsers/operatingsystem

Few TODOs :

  • pkg/term/winconsole see comments below.
  • Add some package documentation

This PR is part of #14756 🐧. It also covers a bit of #11578.

Signed-off-by: Vincent Demeester vincent@sbr.pm

@vdemeester vdemeester force-pushed the pkg-golint branch 2 times, most recently from 7f117e7 to cfd9efc Compare July 20, 2015 16:40
@vdemeester vdemeester changed the title WIP: Lint on pkg/* packages WIP: lint on pkg/* packages (part1) Jul 20, 2015
@vdemeester vdemeester changed the title WIP: lint on pkg/* packages (part1) WIP: lint on pkg/* packages [1/*] Jul 20, 2015
@vdemeester vdemeester force-pushed the pkg-golint branch 2 times, most recently from 63c79c4 to 847c89f Compare July 20, 2015 19:42
@vdemeester vdemeester changed the title WIP: lint on pkg/* packages [1/*] golint on some pkg/* packages Jul 20, 2015

Choose a reason for hiding this comment

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

s/parse/parses/

Choose a reason for hiding this comment

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

The last part of this is a bit difficult to understand. I think I mistakenly gave you a bad recommendation here. Maybe "ParseUnixAddr parses and validates that the specified address is a valid UNIX socket address. It returns a formatted UNIX socket address, either using the address parsed from addr, or the contents of defaultAddr if addr is a blank string".

Totally unrelated to this PR, but ParseHost has a silly line:

addr = fmt.Sprintf("%s", defaultTCPAddr)

Copy link
Member Author

Choose a reason for hiding this comment

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

😻

@aaronlehmann
Copy link

Thanks so much for taking the time to incorporate my feedback. The new diff looks really good. I pointed out a few minor things but it's almost ready to go.

@lowenna
Copy link
Member

lowenna commented Jul 22, 2015

LGTM from a Windows perspective. FYI, #14838 will change the winconsole stuff, so I would hold on that for a separate PR.

@vdemeester
Copy link
Member Author

@jhowardmsft sounds good to me for winconsole :)
@aaronlehmann np 😉, thans so much for taking the time to make such feedback !!

I'm wondering something, which one is better between :

type VersionInfo struct {
    Kernel int    // Version of the kernel (e.g. 4.1.2-generic -> 4)
    Major  int    // Major part of the kernel version (e.g. 4.1.2-generic -> 1)
    Minor  int    // Minor part of the kernel version (e.g. 4.1.2-generic -> 2)
    Flavor string // Flavor of the kernel version (e.g. 4.1.2-generic -> generic)
}

and:

type VersionInfo struct {
    // Version of the kernel (e.g. 4.1.2-generic -> 4)
    Kernel int
    // Major part of the kernel version (e.g. 4.1.2-generic -> 1)
    Major int
    // Minor part of the kernel version (e.g. 4.1.2-generic -> 2)
    Minor int
    // Flavor of the kernel version (e.g. 4.1.2-generic -> generic)
    Flavor string
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the role of a TCPProxy? When should it be used? As a resource, does it require management?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm tempted to say something like on TCPListener (http://golang.org/src/net/tcpsock_posix.go?s=7230:7268#L221) as it's mostly meant to be used as proxy.NewProxy and the function will figure out if it should use TCP or UDP.

As a resource, does it require management?

Not sure at all about that 😓.

@vdemeester vdemeester force-pushed the pkg-golint branch 2 times, most recently from cd89ac7 to 5087924 Compare July 25, 2015 08:26
@vdemeester
Copy link
Member Author

Rebased and added some simple package documentation.

@vdemeester vdemeester force-pushed the pkg-golint branch 3 times, most recently from b7db20d to 29dfa38 Compare July 26, 2015 16:43
@jessfraz
Copy link
Contributor

needs rebase sorry

@jessfraz
Copy link
Contributor

feel free to ping me after for merge

- pkg/useragent
- pkg/units
- pkg/ulimit
- pkg/truncindex
- pkg/timeoutconn
- pkg/term
- pkg/tarsum
- pkg/tailfile
- pkg/systemd
- pkg/stringutils
- pkg/stringid
- pkg/streamformatter
- pkg/sockets
- pkg/signal
- pkg/proxy
- pkg/progressreader
- pkg/pools
- pkg/plugins
- pkg/pidfile
- pkg/parsers
- pkg/parsers/filters
- pkg/parsers/kernel
- pkg/parsers/operatingsystem

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester
Copy link
Member Author

@jfrazelle rebased, let's hope janky is a little happier this time 😝.

@jessfraz
Copy link
Contributor

LGTM effing dev/urandom errors on ci, unrelated

jessfraz pushed a commit that referenced this pull request Jul 27, 2015
golint on some pkg/* packages
@jessfraz jessfraz merged commit 25c42cc into moby:master Jul 27, 2015
@vdemeester vdemeester deleted the pkg-golint branch July 27, 2015 22:20
@vdemeester
Copy link
Member Author

🎉

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.

8 participants