go-tools icon indicating copy to clipboard operation
go-tools copied to clipboard

quickfix: suggest using `slices.Concat` when possible

Open ccoVeille opened this issue 1 year ago • 5 comments

golang 1.22 came with slices.Concat

we could now suggest using it instead of the good old way we had until go 1.22 append + elipsis

slice1 := []int{1, 2, 3}
slice2 := []int{4, 5, 6}

// Concatenating slices
concatenated := append(slice1, slice2...)

// ...

we can now use

slice1 := []int{1, 2, 3}
slice2 := []int{4, 5, 6}

// Concatenating slices
concatenated := slices.Concat(slice1, slice2)

// ...

ccoVeille avatar May 30 '24 17:05 ccoVeille

Personally I wouldn't say that slices.Concat() is strictly better for just two slices:

concat := slices.Concat(slice1, slice2)

concat := append(slice1, slice2...)

Both are easy to read, and both will have the same performance.

However, with three or more slices it's a different story:

concat := slices.Concat(slice1, slice2, slice3)

concat := append(slice1, slice2...)
concat = append(concat, slice3...)
// Or:
concat := append(append(slice1, slice2...), slice3...)

Those are much harder to read. And slices.Concat will typically be faster (whether that matters is of course a different story, but in some cases it likely will).

So I'd say that it should only suggest this for 3 or more slices.

arp242 avatar May 30 '24 17:05 arp242

So I'd say that it should only suggest this for 3 or more slices.

I would say using slices.Concat would somehow promote the usage and curiosity of people to look at

I'm fine with saying it would be suggested only after 3 slices concatenated.

But I have to say. I know the ellipsis when using append confuses some juniore or people coming from other language. Many expect using append(slice1, slice2) to perform what slices.Concat does. But that's another story. Because the suggestion would only occur on code already using the ellipsis.

ccoVeille avatar May 30 '24 18:05 ccoVeille

Well, maybe, but that seems mostly a stylistic preference than anything else. And also:

% rg -g '*.go' -F 'append(' ~/code | grep -F '...' | wc -l
528

And that's just my own projects; doesn't includes stuff from other projects I work on (I put those in ~/src rather than ~/code).

I'm not looking forward to replacing that with something that's basically just identical.

arp242 avatar May 30 '24 18:05 arp242

For the time being I would only add a quickfix that can replace both the 2-slices and n-slices cases. I agree that we don't want to flag all existing code. I also don't want to differentiate between 2 slices and 3+ slices.

Improving existing code would be better suited to go fix (or a tool similar to it.) However, one could also make the argument that mass rewriting of existing code is too noisy and that code should be updated when it gets touched.

dominikh avatar May 30 '24 19:05 dominikh

For the time being I would only add a quickfix that can replace both the 2-slices and n-slices cases. I agree that we don't want to flag all existing code. I also don't want to differentiate between 2 slices and 3+ slices.

Improving existing code would be better suited to go fix (or a tool similar to it.) However, one could also make the argument that mass rewriting of existing code is too noisy and that code should be updated when it gets touched.

I'm fine with this

ccoVeille avatar May 30 '24 19:05 ccoVeille