-
Notifications
You must be signed in to change notification settings - Fork 2
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
Productionization #1
Conversation
Thank you so much, this looks really good :-) |
// need ro run goimports to fix use of fmt/strconv afterwards | ||
} | ||
|
||
case isBasicType(valueType, types.Uint8, types.Uint16, types.Uint32, types.Uint) && oneOf(verb, "%v", "%d"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not just refactoring, but behavioral change.
Now the linter also suggests the change for uint32 and not just uint64, right ?
I know it is faster, but Golang team did not want this for code readability cf discussion in https://go-review.googlesource.com/c/go/+/477675
Could it be optional ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the linter also suggests the change for uint32 and not just uint64, right ?
For any uints, yep
strconv.FormatUint(uint64(ui), 10)
strconv.FormatUint(uint64(ui8), 10)
strconv.FormatUint(uint64(ui16), 10)
strconv.FormatUint(uint64(ui32), 10)
strconv.FormatUint(ui64, 10)
Could you please attach link to specific discussion?
Because I see only discussion about invalid conversions (like int
to int64
), but analyzer do it carefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://go-review.googlesource.com/c/go/+/477675/1..6/src/cmd/go/internal/modfetch/codehost/svn.go#36 comment from Bryan Mills
strconv.FormatInt is ok, although for non-performance-critical code (and particularly for values of types other than exactly int64) I tend to prefer fmt.Sprint instead — the base argument to strconv.FormatInt and the type-conversion (when needed) both somewhat distract from the intent of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
and the type-conversion (when needed) both somewhat distract from the intent of the code
I think if your linter pursues performance goals, then there is no places for such cosmetic and questionable exceptions.
Because, for example, fmt.Sprint(flag)
is more simpler than strconv.FormatBool(flag)
, IMHO.
Could it be optional?
I propose to configure linter behaviour.
E.g.
perfsprint:
enabled:
- string # Just using string
- error # Just using Error()
- bool # strconv.FormatBool
- hex # hex.EncodeToString
- int # strconv.Itoa or strconv.FormatInt
- uint # strconv.FormatUint
int:
# Enable int and int64 only mode
no-type-conversions: true
uint:
# Enable uint64 only mode
no-type-conversions: true
But anyway better if you do it in a separate PR 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have some documentation pointers on how to add this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most popular option is use flags.
And just find inspiration from others linters:
- https://github.com/kulti/thelper/blob/1ed2e864ac8709945d823d882736c3931c178740/pkg/analyzer/analyzer.go#L123 + https://github.com/golangci/golangci-lint/blob/f714c5d842a26b210af714f70f5694ed834c18fb/pkg/golinters/thelper.go#L13
- https://github.com/golangci/golangci-lint/blob/f714c5d842a26b210af714f70f5694ed834c18fb/pkg/golinters/tenv.go#L11 + https://github.com/sivchari/tenv/blob/c014cf9055f973d98ccadf32ef6830081f2530a6/tenv.go#L30
- https://github.com/Antonboom/nilnil/blob/885187c637c0eac882f004311386fff44806b277/pkg/analyzer/analyzer.go#L28 + https://github.com/golangci/golangci-lint/blob/f714c5d842a26b210af714f70f5694ed834c18fb/pkg/golinters/nilnil.go#L16
Or you can support configuration only if linter used with golangci-lint, like
https://github.com/ldez/tagliatelle/blob/72bbdc4749c813be54bf9e3a5eddc18579daa2f7/cmd/tagliatelle/tagliatelle.go#L9 + https://github.com/golangci/golangci-lint/blob/f714c5d842a26b210af714f70f5694ed834c18fb/pkg/golinters/tagliatelle.go#L12
Linters with configuration have ⚙️ icon.
And do not forget update https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml.
P.S. Linter configuration will require more test packages (to test settings logic) 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work
Hello!
I made this work as exception, because found your linter useful and hope to see it in the closest release of golangci-lint.
Please, in the next time be more respectful to your code and neighbour contributors. You contribute into the large open source project, but ignores the basic practises of software development.
TLDR:
main
).Feel free to review and discuss. Thanks