"Specific selector focused" algorithm

[Antonboom]

Related to #1.

---

Hello!

First of all, I want to say that I did not set out to rewrite your project. And of course not every linter gets such a complex review, I just liked your linter idea 🙂

I wanted to show what I meant in my comments. You can accept all or some or nothing. Maybe you can get some inspiration.
The main idea – KISS.

Comments to my commits:

1. Used full message in tests instead of mask. Otherwise it hides bugs, confuses test readers (like me), and prevents contributions to your code (how do I make sure I don't break anything?). critical
   (p.s. if desired, you can go to test generation so as not to escape messages by hand, but use regexp.QuoteMeta)

2. Changed message into a more grammatically correct form (from my point of view). I removed the quotes so that there is no conflict with string literals (otherwise we get escaping in the message). nitpicking, cosmetic

3. Simplified the code to the point of impossibility. As a result, the diagnostric message is now focused on a specific selector. Yes, it produces multiple messages for the chain of getters, but chain looks like a rare case that is still covered by --fix. nitpicking, cosmetic

4. Ignore only protoc-generated files. Why?

• Because sometimes people write custom generators around proto-messages and your linter could be useful in this case.
• Analyzed code should be controled by golangci-lint (look at --skip-dirs-use-default list) by max. Also, you should not parse all the comments in the file, otherwise you can skip a regular file critical

Also:

• removed useless comments (we should write self-document code)
• removed extra pointers

Further comments I will leave in issue.
Thank you for your patience! 💪

[ghostiam]

Hello, thanks for the PR.
Now the linter works a little differently from how it was supposed to work.

Previously, if a correction could be suggested in one line, that was done.
Now we have more unnecessary messages.

Example:

_ = t.Embedded.Embedded 

Previously there was this output:

avoid direct access to proto field t.Embedded.Embedded use t.GetEmbedded().t.GetEmbedded() instead

Now we have 2 different outputs.

avoid direct access to proto field t.Embedded.Embedded, use t.Embedded.GetEmbedded() instead
avoid direct access to proto field t.Embedded, use t.GetEmbedded() instead

Which, in my opinion, creates a lot of noise and makes it difficult to fix manually, and the more access to nested messages, the worse the output will be...

Example

_ = t.Embedded.Embedded.Embedded.Embedded.Embedded.Embedded

test.go:83:6: avoid direct access to proto field t.Embedded.Embedded.Embedded.Embedded.Embedded.Embedded, use t.Embedded.Embedded.Embedded.Embedded.Embedded.GetEmbedded() instead
test.go:83:6: avoid direct access to proto field t.Embedded.Embedded.Embedded.Embedded.Embedded, use t.Embedded.Embedded.Embedded.Embedded.GetEmbedded() instead
test.go:83:6: avoid direct access to proto field t.Embedded.Embedded.Embedded.Embedded, use t.Embedded.Embedded.Embedded.GetEmbedded() instead
test.go:83:6: avoid direct access to proto field t.Embedded.Embedded.Embedded, use t.Embedded.Embedded.GetEmbedded() instead
test.go:83:6: avoid direct access to proto field t.Embedded.Embedded, use t.Embedded.GetEmbedded() instead
test.go:83:6: avoid direct access to proto field t.Embedded, use t.GetEmbedded() instead

Do you have a solution to this problem?

[Antonboom]

Please, reread more carefully my message above.
p.3

[ghostiam]

This is a very common case in my projects, so I do not agree with the statement that the case is rare.
Not everyone uses autofix, so seeing a lot of messages will be annoying for them.
Even I initially launch the linters without autofix, since I once ran into a bug that destroyed my work.

Unfortunately, it's not that simple, which is why I didn't use formatNode initially, since it doesn't give me the control I needed.

[Antonboom]

As you wish.

The most part of PR is suggestion from me, not requirement.
I specially divided it into commits so that you can reuse them easily.

Feel free to close PR and return back to critical points in the checklist.

Or you could try to improve this with chain handling.

You can accept all or some or nothing. Maybe you can get some inspiration

Thanks!

[ghostiam]

Yes, you've given the impetus to rewrite and test it better.
I'll accept your corrections, except the one that breaks the behavior.

Thanks for the good ideas, I'll think about how to change the linter to make the code simpler.

One question: do I understand correctly, in order to accept only 3 out of 4 commits from PR, I just need to cherry-pick?

[ghostiam]

Or is it probably easier to accept all commits and then roll back one of them? I'm just not sure that cherry-peak, github will understand how to accept PR...

[Antonboom]

I separated not-breaking part for you, mate

#3

[ghostiam]

Thank you very much!