feat: Enable additional linter rules just for new changes #1817
Conversation
Signed-off-by: litt3 <[email protected]>
|
The latest Buf updates on your PR. Results from workflow Buf Proto / buf (pull_request).
|
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
This reverts commit 0b70311.
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
litt3
changed the title
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
|
@anupsv This PR removes the separate proxy linting CI action. I think it needs to be removed from the list of |
| - unused # checks for unused constants, variables, functions and types | ||
| - unconvert # removes unnecessary type conversions | ||
| - wrapcheck # checks that errors returned from external packages are wrapped | ||
| - exhaustruct # checks if all structure fields are initialized |
There was a problem hiding this comment.
this I think can be annoying, and also goes against the golang philosophy of not initializing default values?
There was a problem hiding this comment.
golang philosophy of not initializing default values
I feel pretty strongly that this philosophy is a foot gun. We've seen first hand multiple critical bugs that came about because of this!
Essentially, this linter forces you to use raw struct initialization like you'd use a constructor, where each field is a parameter. For cases where you want some default values, you should just create a constructor that accepts any arbitrary parameters, and explicitly configures each struct field internally, IMO
There was a problem hiding this comment.
I think I can agree to this. Even stronger claim is that having "useful zero values" is a mistake (see https://news.ycombinator.com/item?id=26637510) and is one of the reasons why golang will never be able to add an Option type.
One possible counter-argument is if a config struct has a lot of useful default zero values (for example in test), and I want to only change one or two for a specific test. Then if I'm force to add all the other zero values because of this linter, it makes it harder to see which values are actually important/valuable to my test?
There was a problem hiding this comment.
One possible counter-argument is if a config struct has a lot of useful default zero values
Fair point. I think in this case, I still prefer adding test overhead to gain the added safety. There's always the option of making a "default config", which in practice sidesteps this linting rule, and lets you rely on default values.
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
| @@ -16,11 +16,18 @@ jobs: | |||
| steps: | |||
| - name: Checkout EigenDA | |||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 #4.2.2 | |||
| with: | |||
| fetch-depth: 0 # Fetch all history for all branches so golangci-lint can analyze the diff | |||
|
|
|||
| - uses: jdx/mise-action@v2 | |||
There was a problem hiding this comment.
can we use the hash here as well please.
There was a problem hiding this comment.
You mean for mise?
I can open a separate PR for that if you'd like, since it's not related to this linter change.
| @@ -37,7 +37,6 @@ protoc-local: | |||
|
|
|||
| lint: | |||
| golangci-lint run | |||
| go vet ./... | |||
| # Uncomment this once we update to go1.23 which makes the -diff flag available. | |||
| # See https://tip.golang.org/doc/go1.23#go-command | |||
| # go mod tidy -diff | |||
There was a problem hiding this comment.
how about we uncomment this line that I forgot to uncomment and also add it to CI?
There was a problem hiding this comment.
Ok with me doing this in a followup PR, to avoid doubling up on PR purpose?
Signed-off-by: litt3 <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.