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

Some code improvements #868

Merged
merged 1 commit into from
May 6, 2019
Merged

Some code improvements #868

merged 1 commit into from
May 6, 2019

Conversation

sosiska
Copy link
Contributor

@sosiska sosiska commented Apr 30, 2019

  • Rewrite some if-else chains as a switch statements. Based on Go style guide: https://golang.org/doc/effective_go.html#switch
  • Simplify some functions.
  • Improvement of len() comparisons style, because builtin len() function can't return value smaller than zero.
  • strings.ToLower comparison changed to strings.EqualFold.
  • gofmt examples/addsvc/thrift/gen-go/addsvc/add_service-remote/add_service-remote.go file (was done automatically by my IDE ¯\(ツ)/¯ )

@peterbourgon
Copy link
Member

Rewrite some if-else chains as a switch statements. Based on Go style guide: https://golang.org/doc/effective_go.html#switch

This is a style choice, one version isn't better or worse than the other, by default. Unless there is a clear win in legibility, changing these sorts of things is relatively unproductive code churn. I didn't see anything in the diff that would obviously warrant the changes, if you believe otherwise, please give justification.

Improvement of len() comparisons style, because builtin len() function can't return value smaller than zero

These were done deliberately. Please revert.

With those changes, happy to merge.

@sosiska
Copy link
Contributor Author

sosiska commented Apr 30, 2019

These were done deliberately. Please revert.

Why check the obviously impossible option?

if-else chains to switch cases rejected in 6f27a0c

In examples/addsvc/thrift/gen-go/addsvc/add_service-remote/add_service-remote.go file main change on line number 78, len(parsedUrl.Scheme) <= 0 changed to parsedUrl.Scheme == ""

@peterbourgon
Copy link
Member

len returns an int, ints can be negative, <= is safer.

@sosiska
Copy link
Contributor Author

sosiska commented Apr 30, 2019

len returns an int, ints can be negative, <= is safer.

I do not agree, the source code is full of places where the result of the len() is compared with 0.

@peterbourgon
Copy link
Member

If I wrote the code, I used <=. If other people wrote the code, I probably didn't pick any nits about it.

@sosiska
Copy link
Contributor Author

sosiska commented Apr 30, 2019

If I wrote the code, I used <=. If other people wrote the code, I probably didn't pick any nits about it.

https://github.com/go-kit/kit/blame/master/metrics/internal/lv/space.go#L82 ¯\(ツ)/¯

* Simplify some functions.
* strings.ToLower comparison changed to strings.EqualFold.
@sosiska
Copy link
Contributor Author

sosiska commented May 6, 2019

These were done deliberately. Please revert.

Rejected in 40b2404

Copy link
Member

@peterbourgon peterbourgon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Sorry for the delay!

@peterbourgon peterbourgon merged commit fbab14b into go-kit:master May 6, 2019
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

Successfully merging this pull request may close these issues.

2 participants