Fix linting issues #1694
Fix linting issues #1694
Conversation
|
Can we override the patch diff, or should I add test cases for the error checks? |
There was a problem hiding this comment.
Overall I'm very excited about this change! Thank you @skelouse 🎉
- the notes here don't need to be addressed in this PR, but I'd like to get them done soonish
- ignoring the patch-level coverage check is fine in this case, imho
- can either this PR or a near future follow-up PR add
golangci-lintintegration in CI to assert that new linting errors are not introduced?
completion.go
Outdated
| _, err = ctx.App.Writer.Write([]byte(c)) | ||
| if err != nil { | ||
| return Exit(err, 1) | ||
| } |
There was a problem hiding this comment.
This would result in a behavioral change, which is arguably one of:
- a bugfix! ➡️ patch release
- backwards-incompatible ➡️ major release
I'm personally happy to revert this to ignoring the return values because if we can't print completion, something else is probably very unhappy on the system and I don't want to add to the noise by exiting non-zero.
There was a problem hiding this comment.
🤦🏼 this is targeting the v3 alpha, so never mind what I said! Let's exit non-zero on failure to write completion 👍🏼
There was a problem hiding this comment.
Can easily do _, _ for v2 and support linting just the same, good note.
flag_test.go
Outdated
| err := fl.Apply(tfs) | ||
| if err != nil { | ||
| t.Error(err) | ||
| return | ||
| } |
There was a problem hiding this comment.
I'm happy to start using stretchr/testify more around here, e.g.:
| err := fl.Apply(tfs) | |
| if err != nil { | |
| t.Error(err) | |
| return | |
| } | |
| require.Nil(t, fl.Apply(tfs)) |
There was a problem hiding this comment.
Do we want to introduce another import in go.mod? From my perspective it seems that this package has very few external imports. Maybe in another PR as there are many use cases for using testify.
I'd love to make this change, but i'm not sure where to start with CI integrations. |
What type of PR is this?
What this PR does / why we need it:
Fixes 100% of
golangci-lintlinting issuesWhich issue(s) this PR fixes:
Partially fixes #1663
Testing
https://asciinema.org/a/ucDYTAfRZzASFpkmpfdrTxQHJ
ran make
Release Notes