Migrate to golangci-lint v2, fix lint issues #1111
Conversation
7b919eb to
80c024e
Compare
| @@ -179,7 +179,7 @@ func download(version string) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| url := fmt.Sprintf(downloadURL, version, strings.Title(runtime.GOOS)) | |||
| url := fmt.Sprintf(downloadURL, version, strings.ToUpper(runtime.GOOS[:1])+runtime.GOOS[1:]) | |||
There was a problem hiding this comment.
strings.Title() is discouraged because of Unicode stuff, but I didn't feel like pulling in /x/strings just for this simple case.
| @@ -170,6 +170,7 @@ var failingDeployStepErrorMessages = []string{`failed to create file artifact: / | |||
| failed to create file artifact: /bitrise/src/assets: | |||
| failed to get file size, error: file not exist at: /bitrise/src/assets`} | |||
|
|
|||
| //nolint:staticcheck | |||
There was a problem hiding this comment.
Complains about unprintable and non-ASCII characters, but we are testing exactly that.
71a9d1e to
6c415f9
Compare
6c415f9 to
91e521f
Compare
| @@ -3,7 +3,6 @@ package cli | |||
| import ( | |||
| "testing" | |||
|
|
|||
| "github.com/bitrise-io/envman/v2/models" | |||
There was a problem hiding this comment.
nit: Would not the code look nicer if we keep this line and delete the other one?
There was a problem hiding this comment.
I was thinking the same! But then I remembered we also have a models package here in the CLI repo and that's why we introduce the envmanModels and stepmanModels aliases. I changed my mind and think it's less confusing to use envmanModels everywhere, even in files that don't import the other models package. But this is not a hard opinion.
| @@ -24,7 +24,6 @@ import ( | |||
| "github.com/bitrise-io/bitrise/v2/tools" | |||
| "github.com/bitrise-io/bitrise/v2/toolversions" | |||
| envman "github.com/bitrise-io/envman/v2/cli" | |||
| "github.com/bitrise-io/envman/v2/env" | |||
There was a problem hiding this comment.
nit: Similar question here. Keeping this line and delete the other where we override the imported name.
| @@ -3,7 +3,6 @@ package cli | |||
| import ( | |||
| "testing" | |||
|
|
|||
| "github.com/bitrise-io/envman/v2/models" | |||
There was a problem hiding this comment.
nit: Similar as the previous comments, keep this line and delete the other.
| @@ -21,7 +21,7 @@ func StepmanVersion() (version.Version, error) { | |||
| return version.Version{}, err | |||
| } | |||
| if versionPtr == nil { | |||
| return version.Version{}, fmt.Errorf("Failed to parse version (%s)", versionOut) | |||
| return version.Version{}, fmt.Errorf("parse version %s", versionOut) | |||
There was a problem hiding this comment.
Is not the correct error message what was before? Because these errors happen when the stepman/envman/embedd cli version is in an invalid format. S instead of
parse version %s
we could have
invalid version: %s
There was a problem hiding this comment.
I don't have a strong opinion on this, but I prefer these less verbose error strings. The Failed to x thing seems overly verbose when multiple errors are chained.
Checklist
README.mdis updated with the changes (if needed)Version
Requires a MAJOR/MINOR/PATCH version update
Context
Changes
Changes can be reviewed commit by commit.
After migrating the lint config and resolving the new issues, I decided to bite the bullet and fix the old and excluded lint violations too, it wasn't too bad.
Investigation details
Decisions