Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Capital mode - false positive on godoc comments on lower-case variables #22

Open
sbueringer opened this issue Aug 10, 2021 · 5 comments

Comments

@sbueringer
Copy link
Contributor

Hi,

I'm not sure if it's really a false positive. I guess it depends on the definition.

On the following code:

	// gitVersionRegEx matches git versions of style 0.3.7-45-c1aeccb679cd56
	// see ./hack/version.sh for more info.
	gitVersionRegEx = regexp.MustCompile(`(.*)-(\d+)-([0-9,a-f]{14})`)

I get the following finding:

cmd/clusterctl/cmd/version_checker.go:40:5: Sentence should start with a capital letter (godot)
// gitVersionRegEx matches git versions of style 0.3.7-45-c1aeccb679cd56

Afaik godoc comments of lower case variables should start with lower case. Looks like this is already correctly handled for unexported funcs:
https://github.com/tetafro/godot/blob/master/checks.go#L168

@tetafro
Copy link
Owner

tetafro commented Aug 10, 2021

Most probably the problem is that your code is not at the top level. Go provides only one way to get the list of declaration comments: https://github.com/golang/go/blob/master/src/go/ast/ast.go#L1029 . And it's only for top level. That's why other comments are not checked as declaration comments by godot. And that's why you get this error.

This can be fixed just by checking the line after the comment. But it can be quite a big change in code, because currently checkers take only a single comment as an input.

I'll take a look a bit later.

@sbueringer
Copy link
Contributor Author

@tetafro Great, thank you!

Sorry should have added a link to the source with the initial post: https://github.com/kubernetes-sigs/cluster-api/blob/5310648cc15c5170c207eaf4a245e1634aad674c/cmd/clusterctl/cmd/version_checker.go#L40-L42

I think it is top-level? (but as I have absolutely no idea of the AST, this might be wrong)

@tetafro
Copy link
Owner

tetafro commented Aug 11, 2021

Thanks for clarification, I'll check it. But yes, seems like it should be a "top-level" comment.

@tetafro
Copy link
Owner

tetafro commented Sep 5, 2021

I've finally checked it. It's not a declaration comment in terms of go parser. It just doesn't get to go/ast.File.Decls array.

I don't see any way of fixing it without parsing manually source code, which seems too hard, and would significantly increase complexity. So unfortunately this bug will stay unfixed.


JFYI This notation doesn't cause linting errors (once again - because of go parser)

// gitVersionRegEx matches git versions of style 0.3.7-45-c1aeccb679cd56
// see ./hack/version.sh for more info.
var gitVersionRegEx = regexp.MustCompile(`(.*)-(\d+)-([0-9,a-f]{14})`)

@sbueringer
Copy link
Contributor Author

Thank you for looking into this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants