dominikh / go-tools

Staticcheck - The advanced Go linter
https://staticcheck.dev
MIT License
6.01k stars 361 forks source link

Feature Request: Style check for getter methods prefixed with `Get` #1535

Closed dambrisco closed 1 month ago

dambrisco commented 1 month ago

Effective Go suggests that "it's neither idiomatic nor necessary to put Get into the getter's name" (ref: https://go.dev/doc/effective_go#Getters). Adding a style check for this would make this convenient to enforce.

dominikh commented 1 month ago

I don't believe this is something a tool should enforce. It is trivial for a human to catch during code review, and unlike a tool, the human can take into account valid reasons for using the Get prefix, such as interfaces they don't control, avoiding name conflicts, being consistent with a different set of norms, or simply methods that aren't getters but are in fact executing some sort of action that has the nature of getting something.

This is not compatible with Staticcheck's desire not to be wrong.

ccoVeille commented 1 month ago

@dambrisco you may open an issue on revive

They already have a get-return rule https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md

get-return is not about that, but very close in term of linting, I think.

They could accept your proposal. People would be able to enable it or not.

dambrisco commented 1 month ago

@dominikh I appreciate the rationale and I see what you mean. Unlike the other ST checks, the check I've proposed, even in its strictest imaginable form (direct return of matching field), may incorrectly flag correct or necessary implementations. Thanks for the response!

ccoVeille commented 1 month ago

@dambrisco may I ask what you think about my suggestion to open something on revive linter?

I'm unsure if your last message agreeing with @dominikh was like:

dambrisco commented 1 month ago

@ccoVeille I'm not interested in doing so at this time, but I wouldn't discourage you or anyone else from it. dominikh's response forced me to reflect on what I'm actually interested in, and in the end I find myself wholeheartedly agreeing with him.

When it comes to enforcement, the choice in the case where a Get method is declared (and runs counter to the codebase's style) is between:

  1. A reviewer must spot the declaration and must decide whether the name is necessary.
    1. Result: If this guard fails, the function must be renamed.
  2. The author must (in all likelihood) add a linting directive comment, and a reviewer must spot this comment and decide whether the name is necessary.
    1. Result: If this guard fails, the function must be renamed.

As far as I've been able to consider, the outcome is the largely same except that the second option results in an undesirable introduction of permanent linter directive comments.

ccoVeille commented 1 month ago

Thanks for replying me and sharing your thoughts.

I understand why you don't want Togo further now. I share your point of view.

Thanks again