-
Notifications
You must be signed in to change notification settings - Fork 13
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
chore: Update golangci configuration #234
Conversation
Codecov Report
@@ Coverage Diff @@
## master #234 +/- ##
==========================================
- Coverage 97.18% 97.16% -0.02%
==========================================
Files 53 53
Lines 5818 5816 -2
==========================================
- Hits 5654 5651 -3
- Misses 121 122 +1
Partials 43 43
|
I get this when I try to run this locally inside this branch:
I think we want to take care of those deprecations and also remove the now unneeded I see the build is passing, but I found that it might actually be reporting incorrectly. I saw this in the logs (https://github.com/go-vela/types/runs/5116600489?check_suite_focus=true#step:4:76):
I forgot that i modified the yaml prior to running this with the following change: exclude:
- - "^\/\/"
+ - "^\\/\\/" I think that would still ignore comments, but I didn't test to be honest. |
Looking through the list of linters, I found a few that I don't think would hurt to add, but not sure if theres a reason we don't use them... I think there may be value in revisiting these
|
i plan to look through them. i feel better about some of them than others. hoping to check some of their repos to get a better sense. thanks! |
Would the plan be to update all repos with this change once it's in a good place? |
@kneal correct |
seems non-critical? leaning towards no
sure
ok, i think?
i don't think we have a deny list, leaning towards no
i don't know if we want to use this in particular, but i think keeping import order consistent would be nice
i'm good to try it, but a little apprehensive about the noise it might add
i don't mind them personally, however i don't want to keep them forever either, so this would just add noise at the moment, i think
not sure if we're ready for this yet, but i think the premise is fine?
this would be nice to ensure the license header is in place
don't feel strongly on this one, but might be handy in the future?
it's a stylistic thing. i guess consistency is nice. i believe we use both styles currently, but i don't generally favor one over the other myself.
i think it's fine
no, i feel like sometimes we want the flexibility to name imports as whatever and managing this via allow/deny lists is too painful
fine by me
yup
wonder if this would trip us up somewhere.. i'll defer
sure, let's try
sure
seems non-critical? leaning towards no
doesn't
sounds good to me
sounds like there false positives on this, prefer no at this point
sounds like there false positives on this, prefer no at this point |
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 would also like to propose to add wsl
(currently not enabled).. i know it can be noisy, but i like that spacing and selfishly it's nicer to jump around in code (with Vim for example)
btw, thanks for adding comments next to the linters!
.golangci.yml
Outdated
# https://github.com/mdempsky/maligned | ||
maligned: | ||
suggest-new: true |
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.
with maligned
removed, we can get rid of this setting i believe. i think we have to add a govet setting instead to do the fieldalignment
?
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'll add, with reference to wsl above ^^, what about adding nlreturn? -- checks for a new line before return and branch statements to increase code clarity
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.
honestly, i thought wsl
took care of that, but if it doesn't i'm in favor of adding it
- funlen # detects long functions | ||
- goconst # finds repeated strings that could be replaced by a constant | ||
- gocyclo # computes and checks the cyclomatic complexity of functions | ||
- godot # checks if comments end in a period |
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.
not sure how others feel, but i'm personally in favor of removing this one (godot). sometimes it makes you throw a period in odd places
.
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 honestly like godot
I think it does a really good job at ensure consistency with some of the docs. If we didn't have I'm willing to bet we'd have some sentences with periods and some without in the godocs.
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.
k, i mostly don't like it when it forces one when it doesn't make sense, otherwise it's fine
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.
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.
Yeah that's super awkward but I'm guessing the parser is pretty stupid in that case and just looks at the whole comment block. 😢
Taking this updated list and testing it against a few other repos to check for any major issues |
Very true, honestly. So, maybe we put a pin it, with an issue? |
@kneal an issue would be great! i can't promise i will remember otherwise myself :) |
Got you fam go-vela/community#501 |
removed a few that caused too much noise or was just a style preference that we didn't seem to follow. as for
If we don't care about this micro performance enhancement and like the order of the structs as they are, I suggest removing the fieldalignment all together or else we will get a lot of false positives. |
looks like with the current config it wants a space before the return here: Lines 83 to 84 in 850d896
|
Thanks @kaymckay - i'd be interested to hear others' thoughts on |
My thoughts on As far as do we want to use this linter or not in general, I'm on the fence. On the one hand, the linter does offer performance improvements from a memory consumption perspective: https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/fieldalignment On the other hand, I seriously question to what extent we'll see improvements (how much?) from sorting structs 😃 If I had to choose, I guess I'd lean towards using If someone can provide thoughts on why not to use it, I'm all ears because my argument to use it is weak 👍 cc: @kneal |
I know some struct field ordering we likely wouldn't want change, the YAML structs are an example because the way the struct is laid out plays some role in the auto-generation of a YAML file or maybe we don't care. I don't know. |
i think we can summarize that we don't feel strongly about the gains, so let's table it and skip for now :) |
Undoing struct reordering
waiting on approval to proceed with other repos |
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 🐬
Linters we have chosen to disable because we get too many false positives:
Funlen was changed from 100 -> 140 lines 50 -> 70 statements, and excludes comments.
List of added linters:
A reference to all available linters if we want to add more : https://golangci-lint.run/usage/linters/