mgechev / revive

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

enforce-slice-style: Add declaration style #971

Closed jalaziz closed 4 months ago

jalaziz commented 5 months ago

Is your feature request related to a problem? Please describe.

In https://github.com/mgechev/revive/issues/898, it was mentioned:

There is also a way of having a non-initialized slice (which should be preferred over the empty slice w/o len and cap in most cases), but this is beyond the scope of this linter

This preferred way is the var declaration style:

var emptySlice []int

This is the preferred style by both the Golang and Uber style guides:

Describe the solution you'd like

It would be great if the enforce-slice-style rule could be configured to prefer the var declaration style. The tricky part may be that this style is only preferred when declaring a new variable, but other styles may be preferred when assigning an existing variable or passing a parameter.

Describe alternatives you've considered

The only alternative I have considered is disabling the rule.

Additional context

denisvmedia commented 5 months ago

The only alternative I have considered is disabling the rule.

Why disabling the rule is an alternative solution? I think the linter currently does not prevent you from having this:

var emptySlice []int

It just cannot enforce it.

In general, I understand the point here (and I considered it while developing the linter rule), the thing is however is that having an initialized slice (rather than var declaration) is valid for JSON serialization for example. If you enforce var style you won't be able to lint the style for the valid cases (you will have to disable the rule for them).

jalaziz commented 5 months ago

Why is disabling the rule is an alternative solution? I think the linter currently does not prevent you from having this:

It doesn't prevent it, but it does encourage one of two less desirable (and generally "bad" options per popular style guides).

the thing is however is that having an initialized slice (rather than var declaration) is valid for JSON serialization for example

Entirely fair point and not something I had considered. That indeed makes things much trickier, I agree.

I still think it would be nice for the option to exist though. We haven't strictly needed to serialize empty slices as [], and if we did, a one-off exception would be okay.

denisvmedia commented 5 months ago

I guess it should be easy to add: it's basically erroring on both: literal AND make style initialization. Are you willing to submit a PR? 😉

jalaziz commented 5 months ago

Are you willing to submit a PR? 😉

I'm definitely willing to try. I'm not terribly familiar with Golang linting, but willing to learn. Likely can't get to this until the weekend or maybe next weekend, but will try to find time to work on it.

it's basically erroring on both: literal AND make style initialization.

I think it might be more than this because you can't use var style declarations when assigning to a struct member or passing a literal to a function (both of which revive will warn about today).

denisvmedia commented 5 months ago

I think it might be more than this because you can't use var style declarations when assigning to a struct member or passing a literal to a function (both of which revive will warn about today).

Well, you don't need to assign them if it's a brand new instance of the struct or you can assign nil.

jalaziz commented 5 months ago

Well, you don't need to assign them if it's a brand new instance of the struct or you can assign nil.

Ah yeah, fair point. For function argument literals it can still be nice for readability, but certainly shouldn't be needed for structs (although, it could help towards the JSON serialization issue).

Will see what I can figure out when I take a deeper look.

denisvmedia commented 5 months ago

For function argument literals it can still be nice for readability

Even for the function arguments you can do it this way:

func dummy(v []int) {
}

dummy([]int(nil))

This would be consistent with the rule extension you propose.

jalaziz commented 5 months ago

This would be consistent with the rule extension you propose.

Right again. I clearly need sleep 😂. Thanks for the advice!