Skip to content
This repository has been archived by the owner on Feb 21, 2024. It is now read-only.

chore: adds golangci config #2174

Merged
merged 11 commits into from
Oct 17, 2022
Merged

chore: adds golangci config #2174

merged 11 commits into from
Oct 17, 2022

Conversation

CLoZengineer
Copy link
Contributor

No description provided.

@CLoZengineer CLoZengineer marked this pull request as draft October 11, 2022 16:14
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Christopher Lowenthal seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

golangci.yml Outdated
cyclop:
# The maximal code complexity to report.
# Default: 10
max-complexity: 10
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems insane. cyclomatic complexity isn't a great metric at the best of times, but ten? That's very solidly into "this is broken up into so many tiny pieces that reasoning about the tiny pieces becomes overwhelming" territory, I 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.

I think this will come down to a case per case basis.
But I'd like to keep this at the default for now, and we will adjust it as the code quality is tightened up.

golangci.yml Outdated Show resolved Hide resolved
golangci.yml Outdated Show resolved Hide resolved
golangci.yml Outdated Show resolved Hide resolved
golangci.yml Outdated
# Default: false
custom-order: true

gocognit:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we benefit from having both this and the cyclomatic thing? my default preference would be "neither" really.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we're starting this I'd rather maximize signal, and deal with noise later.
Though to your previous comments, I updated this to its default of 30"

golangci.yml Outdated Show resolved Hide resolved
golangci.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@seebs seebs left a comment

Choose a reason for hiding this comment

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

I'm concerned about the sheer volume of new linting here and want some idea what this reports and how much of what it reports is noise. Also, I'd really like to have all the things that say "deprecated, use [...]" be not-used, because that just seems like asking for trouble.

@CLoZengineer
Copy link
Contributor Author

Agree with you @seebs. I just started with the reference and I've been pairing back.
I tend to find that with config files, if you start blank and add, you end up missing a lot of options. But when you pare them away, you're actively thinking of them before you do.

@CLoZengineer CLoZengineer linked an issue Oct 12, 2022 that may be closed by this pull request
@sonarcloud
Copy link

sonarcloud bot commented Oct 12, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@CLoZengineer CLoZengineer marked this pull request as ready for review October 12, 2022 19:39
@@ -1131,8 +1131,7 @@ linters-settings:
- name: context-as-argument
severity: warning
disabled: false
arguments:
- allowTypesBefore: "*testing.T,*github.com/user/repo/testing.Harness"
arguments: []
Copy link
Contributor

Choose a reason for hiding this comment

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

i think it makes sense to have *testing.T in that list, but if we don't happen to actually be using it, that doesn't seem important, we can always fix it later if we care.

Copy link
Contributor

@seebs seebs left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable starting point to me. I'm assuming that this won't result in a situation where we can't merge any changes until we make hundreds of large and intrusive changes, though, if I'm wrong, don't merge it yet. :)

@CLoZengineer CLoZengineer changed the title chore: adds reference golangci config chore: adds golangci config Oct 17, 2022
@CLoZengineer CLoZengineer merged commit 25dbbb2 into master Oct 17, 2022
@CLoZengineer CLoZengineer deleted the chore/golang-ci-config branch October 17, 2022 15:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable golangci-lint
3 participants