Skip to content
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

Update golangci configuration #1785

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

umarcor
Copy link
Contributor

@umarcor umarcor commented Aug 22, 2022

No description provided.

@github-actions github-actions bot added area/github For changes to Github specific things not shipped in the library size/L Denotes a PR that chanes 100-199 lines labels Aug 22, 2022
@marckhouzam
Copy link
Collaborator

Thanks @umarcor, can you provide details on why you had/chose to make these changes? Thanks

@umarcor umarcor force-pushed the umarcor/unused branch 2 times, most recently from 4718c13 to b8690bb Compare August 30, 2022 17:07
@umarcor
Copy link
Contributor Author

umarcor commented Aug 30, 2022

@marckhouzam I just applied the "defaults" as currently documented in https://golangci-lint.run/usage/linters/. I enabled the ones listed in https://golangci-lint.run/usage/linters/#enabled-by-default, which were not enabled here. By the way, I cleaned up the ones marked as "no longer maintained" or "replaced by".

@umarcor
Copy link
Contributor Author

umarcor commented Aug 30, 2022

@marckhouzam also, the makefile was modified because gofmt is now enabled in the golangci-lint configuration. Hence, it is redundant to run it independently.

- varcheck
# Disabled by Default
#- asasalint
#- asciicheck
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize our .golangci.yml had abunch of rules commented out. That seems weird. And makes the linter file harder to read: I don't think anyone really needs to know what rules we explicitly aren't using, that's implied by them not being in the file to begin with.

Can we just remove them?


fmt:
$(info ******************** checking formatting ********************)
@test -z $(shell gofmt -l $(SRC)) || (gofmt -d $(SRC); exit 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we no longer need gofmt because our linter handles it? We should change this to be a call out to the golangci-lint commandline utility then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the 'lint' target in the same Makefile.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha - but is golangci-lint going to format poorly formatted code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. It will check if the format is correct. That's what the current fmt Makefile target does:

cobra/Makefile

Lines 19 to 20 in a281c8b

$(info ******************** checking formatting ********************)
@test -z $(shell gofmt -l $(SRC)) || (gofmt -d $(SRC); exit 1)

This PR does not introduce any change in that regard.


default: all

all: fmt test
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing fmt here means that when people run make locally, they will no longer get errors for formatting errors. Considering the linter takes less tan 2 seconds to run, I suggest running it for make all.
If someone does not want to run it, they can do make test directly.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fmt is removed here because the Makefile target fmt is removed in this PR. We can add lint in this target, but that will require contributors to have golangci-lint installed before running make. I think it is better not to require having golangci-lint in order to make cobra. At the same time, it does not make sense to have an specific make target for one single linter but not for others. Overall, the users/developers interested in contributing code will always want to make lint test clean.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I started contributing I would do make and think everything was ok. It was only later that I realized this did not include the linters, so now I run make lint manually.

But I agree that making golangc-lint a requirement may be annoying. But it will also be annoying to only notice the bad formatting once CI is run...

I'm not sure what is best...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO modifying the sources and building cobra are not directly related to contributing code back to the upstream. That is, users might want to use and modify this open source tool regardless of the linting rules enforced in this repository. From this point of view, the requirement/need to use golangci-lint belongs to contributing guidelines: https://github.com/spf13/cobra/blob/main/CONTRIBUTING.md. Everyone should read the contributing guidelines before submitting a PR to a project.

@github-actions github-actions bot removed the area/github For changes to Github specific things not shipped in the library label Dec 14, 2022
@umarcor umarcor force-pushed the umarcor/unused branch 2 times, most recently from 27f6105 to be6cd55 Compare June 20, 2023 18:04
@umarcor umarcor marked this pull request as draft June 20, 2023 18:05
@umarcor umarcor force-pushed the umarcor/unused branch 2 times, most recently from 22f06f4 to 567f40f Compare July 18, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that chanes 100-199 lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants