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

Generated code can't pass staticcheck #26

Closed
sunyakun opened this issue Jan 11, 2022 · 4 comments
Closed

Generated code can't pass staticcheck #26

sunyakun opened this issue Jan 11, 2022 · 4 comments
Assignees
Labels
style Styles, coding convention and formating issues

Comments

@sunyakun
Copy link

Description

thriftgo generated code can't pass staticcheck, so we must write an extra config file staticcheck.conf to ignore some checks, but staticcheck will ignore all golang code files under the same directory tree which the config file placed

Reproduction

assume we have a thrift file named 'foo.thrift' and below is the content

namespace golang foo

struct Foo {
    1: required string name,
    2: required i64 age,
}

execute shell commands below

go mod init code.byted.org/demo
thriftgo -g go -o ./ foo.thrift
go mod tidy
go mod edit -replace  github.com/apache/thrift=github.com/apache/thrift@v0.13.0
go mod tidy

staticcheck ./...

and will get the output

foo/foo.go:113:67: error strings should not be capitalized (ST1005)

Recommandation

  1. add //lint:ignore directive to generator/golang/templates/struct.go::StructLikeRead
RequiredFieldNotSetError:
	//lint:ignore ST1005 {some reasons}
	return thrift.NewTProtocolExceptionWithType(thrift.INVALID_DATA, fmt.Errorf("Required field %s is not set", fieldIDToName_ModelSync[fieldId]))
  1. use lowercase word instead
return thrift.NewTProtocolExceptionWithType(thrift.INVALID_DATA, fmt.Errorf("required field %s is not set", fieldIDToName_ModelSync[fieldId]))

Reference

@lsjbd
Copy link
Contributor

lsjbd commented Jan 12, 2022

That's awkward. Files generated by thriftgo all contains a header comment // Code generated by thriftgo (x.y.z). DO NOT EDIT. and staticcheck should ignore them as most linters do. Can you check if this is an issue of staticcheck ?

@sunyakun
Copy link
Author

This is a known issue of staticcheck, but the author recommends 'generator' to fix the problems, and I also find a code review comment about error string: https://github.com/golang/go/wiki/CodeReviewComments#error-strings, so does this is a bad smell of thriftgo or there have some reasons to use capitalized in error string?

@lsjbd
Copy link
Contributor

lsjbd commented Jan 12, 2022

OK, we will fix the error string issue in the next version.

But after all, staticcheck does not follow the common way for ignoring generated files but asks all potential generators to meet its requirement. I think this is irrational.

And, DO NOT EDIT is recommended by the go officials. See golang/go#41196.

@lsjbd lsjbd added the style Styles, coding convention and formating issues label Jan 12, 2022
@lsjbd lsjbd self-assigned this Jan 12, 2022
@lsjbd
Copy link
Contributor

lsjbd commented Feb 18, 2022

This issue has been solved in v0.1.4.

@lsjbd lsjbd closed this as completed Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
style Styles, coding convention and formating issues
Development

No branches or pull requests

2 participants