All proxy functions/receivers need to be documented.

clearcontainers / proxy

Hypervisor based containers proxy

Apache License 2.0

32 stars 15 forks source link

All proxy functions/receivers need to be documented. #13

Open dlespiau opened 7 years ago

dlespiau commented 7 years ago

From @jodh-intel on October 26, 2016 7:57

Related to #348 and original comments added to PR #306.

Copied from original issue: 01org/cc-oci-runtime#350

dlespiau commented 7 years ago

does it make sense to you to downgrade this with "all exported functions"? I think that's what goling checks for. I don't think having a header for all function is really necessary, many little functions are self-documenting.

dlespiau commented 7 years ago

PR #360 has a compromise: have golint be happy, ie all exposed symbols are documented. This can be abused a bit a main packages, hiding all symbols in those.

A potential way to limit that abuse in the future could be to restrict the main package to have nothing else but the main function and no object implementation. The package view would look like:

./proxy: the proxy package where all objects currently defined in the main package are defined and exposed (Proxy, Vm, Protocol, ....)
./proxy/daemon: the main package with one file with the main function
./poxy/api: like today

dlespiau commented 7 years ago

@jodh-intel golint passes and now enforces that every exposed symbol must be documented. I think that's an acceptable trade-off and certainly widely accepted in other go project.

Happy to close this bug?

dlespiau commented 7 years ago

From @jodh-intel on November 10, 2016 17:33

There still seem to be public functions that are not documented (for example protocol.Serve()).

jodh-intel commented 7 years ago

I still don't see doc for protocol.Serve et al. I'm not clear why golint is happy with protocol.go as a result. I'm assuming it's something to do with the fact that the types specified in that file are private, yet some of the receives are public?

dlespiau commented 7 years ago

golint is happy because they are in the main package, not a package that can be imported from elsewhere.

jodh-intel commented 7 years ago

I can see that the code passes golint, but the behavior of that tool feels marginally sub-optimal to me: just because a method/receiver is in the main package should not mean it gets to be "comment-header-less" in my view.

@sameo, @grahamwhaley - what are your thoughts on this?