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

removing bad newline #266

Merged
merged 1 commit into from
Nov 26, 2020
Merged

removing bad newline #266

merged 1 commit into from
Nov 26, 2020

Conversation

Zamiell
Copy link
Contributor

@Zamiell Zamiell commented Nov 25, 2020

this seems like a pointless newline, correct me if I am wrong.

furthermore, I would recommend using wsl in CI to automatically detect these kinds of errors in the future.
wsl is included in golangci-lint, which is the defacto golang linter, which it seems like you don't use currently.

@Zamiell Zamiell requested a review from nhooyr as a code owner November 25, 2020 20:09
@@ -16,7 +16,6 @@ import (
// It ensures the client speaks the echo subprotocol and
// only allows one message every 100ms with a 10 message burst.
type echoServer struct {

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch!

@nhooyr
Copy link
Contributor

nhooyr commented Nov 26, 2020

furthermore, I would recommend using wsl in CI to automatically detect these kinds of errors in the future.
wsl is included in golangci-lint, which is the defacto golang linter, which it seems like you don't use currently.

I prefer to avoid small dependencies. idt this is common enough a bug to justify incorporating wsl.

I disagree that golangci-lint is the defacto go linter.

It's goimports, gofmt, golint and go vet all of which we use and are from the Go team.

@nhooyr nhooyr merged commit c9f314a into coder:master Nov 26, 2020
@Zamiell
Copy link
Contributor Author

Zamiell commented Nov 27, 2020

  1. Well, to be technical, goimports and gofmt are code formatters, not linters. =p
  2. golangci-lint includes goimports, gofmt, golint, govet, as well as various other useful linters, like the previously mentioned wsl.
  3. Note that golint is now officially deprecated and not very many people use it anymore: x/lint: freeze and deprecate golang/go#38968

For my personal Golang projects, I've found a tremendous amount of value in using golanci-lint. If you want to keep it simple and not use it, that's fine - I just thought I would throw it out there for consideration.

golanci-lint has some linters enabled by default and some disabled by default. My typical workflow is that I enable every single linter that it offers. This generates a lot of noise, so then I go through and manually disable the ones that I disagree with, and the ones that are noisy. After that, I am left with a few legitimate issues / bugs in my code, which I clean up, making the whole process worth it. =)

An example yaml config file:
https://github.com/Zamiell/hanabi-live/blob/master/server/.golangci.yml

@nhooyr
Copy link
Contributor

nhooyr commented Nov 27, 2020

Well, to be technical, goimports and gofmt are code formatters, not linters. =p

Fair but the line has grown ever thin. code formatting isn't far from automated linting imo.

Note that golint is now officially deprecated and not very many people use it anymore: golang/go#38968

That's wild and definitely puts a dent in golint for me. I've found it extremely useful, I don't really understand the motivation for removing it in that thread and fully agree with golang/go#38968 (comment)

For my personal Golang projects, I've found a tremendous amount of value in using golanci-lint. If you want to keep it simple and not use it, that's fine - I just thought I would throw it out there for consideration.

Yea I appreciate the thoughts and discussion but for now I'm definitely in favour of keeping things as minimal as possible.

Perhaps I'll switch the library to Staticcheck which seems like it's highly recommended from that thread and a sort of successor to golint.

nhooyr added a commit that referenced this pull request Jan 9, 2021
nhooyr added a commit that referenced this pull request Jan 9, 2021
There were a few PRs merged into the master branch that were then not
merged into the dev branch. This branch merges those changes in cleanly.

- #261
- #266
- #273
@nhooyr nhooyr mentioned this pull request Jan 9, 2021
nhooyr added a commit that referenced this pull request Jan 9, 2021
There were a few PRs merged into the master branch that were then not
merged into the dev branch. This branch merges those changes in cleanly.

- #261
- #266
- #273
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