#110 - Fix Slice bug producing a \n when provided with an empty slice

[hsheikhali1]

see #110 for discussion

Resolved a minor bug where Slice was returning an newline when an empty string slice was passed into it.

The expected behaviour:

// Slice creates a pipe containing each element of the supplied slice of
// strings, one per line

result, err := script.ListFiles("/empty/path/")

// result returns ""

The Actual Behaviour:

result, err := script.ListFiles("/empty/path/")

// result returns "\n"

Use Case for PR

This MR will add logic that will tell Slice to check the length of the incoming string slice (len([]string{ ... })) and if it's less than or equal to 0, or if it's empty, it should tell Slice to return a pipe containing an empty string rather than a "\n"

Slice is used often in the script codebase particularly in ListFiles() where the bug was first discovered. This implementation should resolve it.

[bitfield]

Oh dear, apparently I forgot to submit this review. Sorry!

[bitfield]

It seems unlikely that len(s) could be less than zero, doesn't it?

Clearly the special case here is where the length equals zero, but it seems a shame to duplicate virtually the whole function for this case. Actually, the difference in behaviour is whether or not we add a final newline. It seems slightly neater to me to isolate just that in the special-case branch.

[bitfield]

And if s has zero elements, there's no need to call strings.Join in any case: there's nothing to join.

Instead, what about:

if len(s) == 0 {
	return NewPipe()
}
return Echo(strings.Join(s, "\n") + "\n")

[hsheikhali1]

That's a very good point ! @bitfield thanks!

[bitfield]

This is great, but it's house style with this project to use no blank lines within functions. To my mind, they just make the function longer without providing any benefit.

[hsheikhali1]

This is a thing I have built into muscle memory from my typescript work.. I noticed some Go devs prefer not to have a bunch of blank lines

[bitfield]

Actually, since the result of Slice is a Pipe, maybe we should say something like:

func TestSliceGivenEmptySliceProducesEmptyPipe

[bitfield]

And we could probably use an extra test here for the non-empty slice behaviour, couldn't we? (The fact that we don't have one is entirely on me, but this is a good chance to add it.)

[hsheikhali1]

Doesn't this test cover non-empty slice behaviour ?

func TestSliceProducesElementsOfSpecifiedSliceOnePerLine(t *testing.T) {
	t.Parallel()
	want := "1\n2\n3\n"
	got, err := script.Slice([]string{"1", "2", "3"}).String()
	if err != nil {
		t.Fatal(err)
	}
	if !cmp.Equal(want, got) {
		t.Error(cmp.Diff(want, got))
	}
}