Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
relloyd opened this issue Apr 18, 2022 · 9 comments · May be fixed by #179
Open

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

relloyd opened this issue Apr 18, 2022 · 9 comments · May be fixed by #179

Comments

@relloyd
Copy link

relloyd commented Apr 18, 2022

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()
@relloyd relloyd changed the title ListFiles() Produces an Unwanted Newline Character ListFiles() Produces an Unwanted Newline Character When Zero Files Are Matched Apr 18, 2022
@bitfield
Copy link
Owner

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
Copy link

hsheikhali1 commented May 1, 2022

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
Copy link

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
Copy link
Owner

bitfield commented May 2, 2022

Absolutely, @hsheikhali1. Please do!

@hsheikhali1
Copy link

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

@hsheikhali1
Copy link

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

@bitfield
Copy link
Owner

bitfield commented May 8, 2022

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
Copy link

hsheikhali1 commented May 8, 2022

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
Copy link

MrTbag commented Jul 13, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants