search

mgechev / revive

🔥 ~6x faster, stricter, configurable, extensible, and beautiful drop-in replacement for golint
https://revive.run
MIT License
4.73k stars 276 forks source link

Linter rule to check the consistency of make(map[type]type) vs empty map[type]type{} #892

Closed denisvmedia closed 11 months ago

denisvmedia commented 11 months ago

Is your feature request related to a problem? Please describe. There are two ways of declaring a map: either via make or via literal. Some projects might want to require to be consistent by choosing this or that way of map declaration.

Describe the solution you'd like I'd like to let the user choose between the styles (either make or via literal). make(map[type]type, size) will always be allowed, as well as map[type]type{k1: v1, k2: v2}. I'm also considering an option to check or ignore map descendant types (e.g. type Map map[string]string), on by default.

Describe alternatives you've considered I didn't find any solution.

Additional context IMO, consistent code, be it opinionated or not, is better than inconsistent. Also, I'm in favor for make(map[type]type) because it's then consistent with make(map[type]type, size). Also, when a descendant type is declared and then used, it's not clear for the reader if it's a struct or a map, when we call it as Map{}.

P.S. I can implement such a linter rule, if it makes sense to the others.

denisvmedia commented 11 months ago

@chavacava there was no reaction for a couple of days - do you think it's not reasonable to have in revive?

chavacava commented 11 months ago

Hi @denisvmedia, thanks for the proposal (and sorry for the late response, I was off these days) I think "style" enforcing rules have their place in revive; thus it could be the case of the rule you propose. Said that, I'm not sure about the utility or pertinence of this particular rule. Both declaration styles (with make and with a literal) are not exactly equivalents (while make(map[type]type, size) will set an user defined capacity for the map the other, map[type]type{}, will use a default capacity) therefore they serve (slightly) different purposes and both should be accepted in a code base. If the rule goal is to enforce make(map[type]type) over map[type]type{} (two semantically equivalent expressions) then it could be okay. It is, of course, possible that I'm not completely understanding the proposed rule. If that is the case, please provide examples.

denisvmedia commented 11 months ago

If the rule goal is to enforce make(map[type]type) over map[type]type{} (two semantically equivalent expressions) then it could be okay.

This was exactly my intention.

chavacava commented 11 months ago

In that case you can go on with a PR. Please remember, the rule will be automatically enabled on configurations with enableAllRules = true therefore, its default behavior must not produce too much noise.

denisvmedia commented 11 months ago

I see. I'm not sure how much noise it will produce if it's exactly about make(map[type]type) over map[type]type{}. Maybe it can have no defaults and only by enabling this or that style it will start real linting. What do you think?

chavacava commented 11 months ago

I usually test new rules over open source code bases (Kubernetes, docker, hugo, ...) These test provide a first idea on the number of issues the rule will find on real code (and how well it supports weird/unusual/unpredicted code structures) So you could develop a fist version of the rule, test it on these kind of code bases and then adjust rule's configuration and defaults.