ListFiles() Produces an Unwanted Newline Character When Zero Files Are Matched

[relloyd]

In the following example pipe, if the glob matches zero files, then a single empty line is supplied to ExecForEach(), which causes the rm command to error:

_, err := script.ListFiles("PY*").ExecForEach("rm -v {{.}}").Stdout()

Can ListFiles() only produce an output if files are matched?

There is a rather ugly workaround that filters blank lines as follows:

_, err := script.ListFiles("PY*").RejectRegexp(regexp.MustCompile("^$")).ExecForEach("rm -v {{.}}").Stdout()

[bitfield]

Thanks, @relloyd! Arguably, this is a bug in Slice, which is used to return the results of ListFiles. If the slice is empty, the resulting pipe should contain no lines. PRs welcomed!

[hsheikhali1]

Thanks, @relloyd! Arguably, this is a bug in Slice, which is used to return the results of ListFiles. If the slice is empty, the resulting pipe should contain no lines. PRs welcomed!

return Echo(strings.Join(s, "\n") + "\n")

probably don't want to return new lines here if the string is empty right?

[hsheikhali1]

I wouldn't mind opening a PR for this if there isn't one :) I'm rather new to go so this is a perfect learning opportunity for me

[bitfield]

Absolutely, @hsheikhali1. Please do!

[hsheikhali1]

Okay I was finally able to sit down and do some work for this :) I should have an PR soon

[hsheikhali1]

So @bitfield - I have a newbie question lol. What's the best approach to testing my implementation of this fix?

[bitfield]

That's a very good question @hsheikhali1! Maybe one way to approach this is to ask "When my code is correct, what behaviour would users see from their programs?"

We might answer that by saying, for example, "Calling Slice on a pipe with no data will produce a slice with zero elements". When you phrase it that way, it's relatively easy to imagine translating this into the form of a Go test, isn't it? And we'd want to see that with the current version of script, the test fails (in the expected way). Then, when you apply your changes, the test should start passing.

We can also imagine, though, a malicious implementation of Slice that just always returns an empty slice, no matter what! To catch this, we should also have some other tests, or test cases: a pipe with one line of data produces a slice of one element, a pipe with two lines produces two elements, and so on. There are always more cases you could test, but zero, one, and two should be enough to give a decent level of confidence in the system.

[hsheikhali1]

That's a very good question @hsheikhali1! Maybe one way to approach this is to ask "When my code is correct, what behaviour would users see from their programs?"

We might answer that by saying, for example, "Calling Slice on a pipe with no data will produce a slice with zero elements". When you phrase it that way, it's relatively easy to imagine translating this into the form of a Go test, isn't it? And we'd want to see that with the current version of script, the test fails (in the expected way). Then, when you apply your changes, the test should start passing.

We can also imagine, though, a malicious implementation of Slice that just always returns an empty slice, no matter what! To catch this, we should also have some other tests, or test cases: a pipe with one line of data produces a slice of one element, a pipe with two lines produces two elements, and so on. There are always more cases you could test, but zero, one, and two should be enough to give a decent level of confidence in the system.

For more context I've already written what I think the solution should look BUT this answer you've given has got me thinking. Thank you so much for the insightful comments

[MrTbag]

I opened a PR after making quite some changes to the ListFiles() method after trying to look at it in a bit of a different way and used os.Stat to check whether the path argument is referring to a file or not.
Just tell me if I have to change the code based off of your preferences; otherwise the problem should be solved now.

[mahadzaryab1]

@bitfield What's the status of this? Does it still need to be fixed?

[bitfield]

I think I rather dropped the ball here on #110. Could you please review the situation and see where we are? I certainly think Slice should return an empty pipe for an empty slice, but I'm not sure about the implications of the change.

[mahadzaryab1]

@bitfield it looks like there's an open PR ready for your review at #115

[bitfield]

Yes, #115 is what I meant, sorry. Would you like to take a look at it and bring it up to date and address any outstanding comments? Alternatively, implement it from scratch if you prefer.

[mahadzaryab1]

@bitfield PR ready for review at #216.