Fix Makefile install instructions and improve CONTRIBUTING.md #1823
Conversation
|
|
|
@FollowTheProcess can you please rebase on top of #1785 ? |
.golangci.yml
Outdated
| #- lll | ||
| - maligned | ||
| # - maligned |
There was a problem hiding this comment.
I don't think we are running the replacement linter for this one.
According to the golangci-lint warning: "Replaced by govet 'fieldalignment'"
If I make the following modification without the PR I get an linter error, but not after the PR.
diff --git a/command.go b/command.go
index 9d5e9cf..943bb17 100644
--- a/command.go
+++ b/command.go
@@ -175,9 +175,6 @@ type Command struct {
// helpCommandGroupID is the group id for the helpCommand
helpCommandGroupID string
- // completionCommandGroupID is the group id for the completion command
- completionCommandGroupID string
-
// versionTemplate is the version template defined by user.
versionTemplate string
@@ -194,6 +191,8 @@ type Command struct {
// CompletionOptions is a set of options to control the handling of shell completion
CompletionOptions CompletionOptions
+ // completionCommandGroupID is the group id for the completion command
+ completionCommandGroupID string
// commandsAreSorted defines, if command slice are sorted or not.
commandsAreSorted bool
// commandCalledAs is the name or alias value used to call this command.
There was a problem hiding this comment.
Yeah I think we'd need to configure govet to do that: https://golangci-lint.run/usage/linters/#govet. I tend to just use:
govet:
enable-all: trueBut if we've not run that before it might trigger a load of stuff
Ahh sorry I didn't see there was already one addressing the linter config. If it's easier and keeps this PR cleaner I can remove the linter changes from this one? |
Yeah, if we keep the changes separate, we will be able to merge more easily. Thanks 👍 |
FollowTheProcess
force-pushed
the
chore/makefile
branch
from
161eb0d to
2e8acad
Compare
Makefile
Outdated
|
|
||
| ifeq (, $(shell which golangci-lint)) | ||
| $(warning "could not find golangci-lint in $(PATH), run: curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh") | ||
| $(warning "could not find golangci-lint in PATH, run: curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH)/bin v1.50.0") |
There was a problem hiding this comment.
My only concern here is that the command now specifies a version of the linter to install (which would have to be kept up to date). Maybe it's better to simply point to the website which has good install docs: https://golangci-lint.run/usage/install/
There was a problem hiding this comment.
My only concern here is that the command now specifies a version of the linter to install (which would have to be kept up to date). Maybe it's better to simply point to the website which has good install docs: https://golangci-lint.run/usage/install/
Agreed.
There was a problem hiding this comment.
An alternative is to use go install, as done in CI. However, that won't add it to the PATH.
|
The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:
|
|
Please don't stale 😂 do I need to do anything to help you guys be able to merge this? |
|
@FollowTheProcess, see #1858. You can ignore those comments. Furthermore, you might want to propose a PR to remove the stale-bot. Removing https://github.com/spf13/cobra/blob/main/.github/workflows/stale.yml should suffice. |
Closes #1820
This PR addresses the concerns raised in the linked issue above re. Makefile printing entire
$PATHvariable and incorrect instructions for installing golangci-lint.I've also added a line in
CONTRIBUTING.mdlinking to the two dev tools required.On running linting there was a few warnings about deprecated and/or abandoned linters so I've fixed them up:
I've made the suggested replacements (most were replaced by
unused)