-
Notifications
You must be signed in to change notification settings - Fork 1
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
Review in context of golangci-lint integration #1
Comments
Thank you very much for the review, I will make corrections as soon as possible.
That is, from this test := many[0].Embedded.S == "" || t.Embedded.CustomMethod() == nil || t.S == "" || t.Embedded == nil to this
will be obtained and if I leave only one
...
In theory, if there is a false positive, then it will be in some unusual cases, since only fields that have a field with the |
@Antonboom if you have the opportunity, could you review it again? 1 - Renamed to 4 - Now the messages look like this:
6 - Fixed. 8 - Fixed and also added new checks that did not pass in k8s. 9 - I added a detailed comment and found a place for optimization. 10 - Fixed? k8s results
|
@ghostiam, thank u for work! Checklist:
Also, some my thoughts.
But I'm not sure we need to check it, or need to configure to ignore such cases. It looks like consistency for the entire project is good anyway. And for everything else we have 👌 |
1 - We can't use https://pkg.go.dev/go/types#Implements because it would make us depend on 3 -
4 - The linter will ignore this case anyway, since access to fields is not used there. Also, gRPC allows you not to fill out the entire message, you can send it empty. |
Feel free to split in separate issues if needed.
@ghostiam, hi!
Thank you for linter.
I deeply reviewed it.
Naming proposal –
protogolint
is too general and conflicts with protolint and similar. If in the future will appear the new proto-related linter when we going to question – why is it not part of your linter? IMHO, better to narrow naming to specific functionality. In your case –protoaccess
,protoderefchecker
,protogetter
,protodeferlint
,protousegetter
,protoshield
,protonodirectread
,protoreadguard
(help me 🙂). Inspiration can be drawn from https://golangci-lint.run/usage/linters/// want "proto field read without getter:"
What is purpose of
:
in the end?Based on implementation looks like a bug / typo.
// want "proto field read without getter:" "proto field read without getter:" "proto field read without getter:" "proto field read without getter:"
IMHO, it is cool to merge the similar messages (example) or make it more informative (see below).
It is cool to make message more informative and without tautology, e.g.
(help me 🙂)
Great idea with different suggested fixes modes:
My respect!
You wrote
but dont use this dependency.
You ignore linting for linter project (see check list).
E.g. I see preallocation cases.
IMHO, try to avoid strings comparison like
(but in this case I cannot suggest better approach)
Also, old projects uses
github.com/golang/protobuf
.In such cases it doesn't exist?
IMHO
is not the best naming, it's more about suggested fixing, not checking itself.
or
Checker
type itself just does not match the letter S from SOLID.In addition
check
looks over-engineered a lot.I expected simple logic like:
SelectorExpr
go/format.Node
Reporting linter errors via diagnostic doesn't look as common and nice practice
(@ldez knows better)
🙂
P.S. Run linter in k8s and look nice results and messages 👍
No panics.
Expand me
But I am not sure about false positives.
The text was updated successfully, but these errors were encountered: