Conversation
|
|
||
| dir := filepath.Dir(path) | ||
| if _, ok := packages[dir]; !ok { | ||
| // If this file has a package-level doc, mark the package as |
There was a problem hiding this comment.
What happens if two files have package-level docs? Should we make sure that doesn't happen, or is it okay?
| // documented. | ||
| if node.Doc != nil { | ||
| packages[dir] = true | ||
| } else if !packages[dir] { |
There was a problem hiding this comment.
I don't understand what this else clause is doing - it looks (to me) like it's saying "if something is false, make it false" but my guess is that I'm missing something subtle.
There was a problem hiding this comment.
Ah, I think I get it - it's adding a false value if the map entry doesn't already exist, right?
(I think there's something else odd at the moment, as I've got some packages where I've written documentation but it's not picking it up. Looking more closely...)
There was a problem hiding this comment.
I think we just want to remove the outer condition, i.e. just make this
// If this file has a package-level doc, mark the package as
// documented. Otherwise, if we haven't seen the package before,
// remember it as undocumented.
if node.Doc != nil {
packages[dir] = true
} else if !packages[dir] {
packages[dir] = false
}... as otherwise, only the first file that we come across is relevant to whether the package is deemed to be documented or not.
There was a problem hiding this comment.
t looks (to me) like it's saying "if something is false, make it false"
It is confusing to write it that way. Usually we don't distinguish between false and not present in map[T]bools, but here we're doing that. I clearer way would use the "presence" bit like so:
if _, ok := packages[dir]; !ok {
packages[dir] = false
}
However, to your point about multiple files, I think this would be improved by using a map from string to []string. Record all the files with package-level comments, and then any length other than 1 is problematic. And for n > 1, you can print all the filenames.
| } | ||
|
|
||
| // Verify each package has documentation. | ||
| for pkgPath, documented := range packages { |
There was a problem hiding this comment.
This needs to be moved outside the WalkDir call, otherwise we report the currently-undocumented packages on each file. (This panicked me briefly, until I worked out what was going on...)
|
The generated code for in statepb doesn't contain any doc comments. @codyoss might know of a way of changing that, or we could explicitly skip |
|
Closing per CONTRIBUTING.md, since I won’t have time to fix this today. I’ll reopen later in the week. |
3 participants
Update TestDocComments to check that each Go package includes a top-level doc comment.
For #476
Fixes #522