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
Add gocritic to linters #285
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly one of the rule is to not use a package name as a variable one, I'm ok with that only if we don't lose the meaning of the variable in our code base, my2cents
loader/loader.go
Outdated
@@ -664,16 +673,16 @@ func resolveVolumePath(volume types.ServiceVolumeConfig, workingDir string, look | |||
} | |||
|
|||
// TODO: make this more robust | |||
func expandUser(path string, lookupEnv template.Mapping) string { | |||
if strings.HasPrefix(path, "~") { | |||
func expandUser(p string, lookupEnv template.Mapping) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry but I definitely not a fan of changing path
to p
to make a linter happy, we lost to much meaning of the function argument in that case!
Isn't possible the alias the import instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I'm just not having the inspiration for a good name 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed the import to paths
906106b
to
2e30acb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 ♻️
loader/validate.go
Outdated
@@ -38,6 +38,12 @@ func checkConsistency(project *types.Project) error { | |||
} | |||
} | |||
|
|||
for dependedService := range s.DependsOn { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Threw in a freebie, eh? Normally I'd say separate PR but it's a small change so 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! Good catch! But I didn't add that 😮
It's already on master
https://github.com/compose-spec/compose-go/blob/master/loader/validate.go#L41-L45
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just rebased and it seems to remove this section from the diff
Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
- diagnostic - opinionated - style Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
Reference -> https://golangci-lint.run/usage/false-positives/#nolint-directive Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
621de48
to
8676d27
Compare
No description provided.