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

testing: example output whitespace trimming can be confusing #23542

Closed
josharian opened this issue Jan 24, 2018 · 15 comments
Closed

testing: example output whitespace trimming can be confusing #23542

josharian opened this issue Jan 24, 2018 · 15 comments
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@josharian
Copy link
Contributor

josharian commented Jan 24, 2018

Consider the Example for filepath.Split.

The code is:

fmt.Println(path.Split("static/myfile.css"))
fmt.Println(path.Split("myfile.css"))
fmt.Println(path.Split(""))

The output is:

static/ myfile.css
 myfile.css

Given that there are three fmt.Println calls, it seems to me that there really ought to be three lines of output.

(And if we decide that we don't want to or can't change example output whitespace trimming, we should at least tweak this example so that the last line of output contains non-whitespace, e.g. by prefixing each output line with an asterisk or number.)

@josharian josharian added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 24, 2018
@josharian josharian added this to the Go1.11 milestone Jan 24, 2018
@mvdan
Copy link
Member

mvdan commented Jan 25, 2018

Perhaps another solution would be to not leave the empty line print at either end, so that it could not be trimmed. Leaving it in the middle, for example.

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 25, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 25, 2018
@bradfitz bradfitz added Documentation help wanted NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 25, 2018
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Jan 25, 2018
@awoodbeck
Copy link
Contributor

awoodbeck commented Jan 29, 2018

I could take this one. I prefer mvdan's suggestion unless we'd like to be more explicit with the whitespace.

Output would of course look like this:

static/ myfile.css
 
 myfile.css

Edit: Here's another proposal with explicit treatment of whitespace:

The code:

d, f := path.Split("static/myfile.css")
fmt.Printf("path.Split(%q) = %q %q\n", "static/myfile.css", d, f)

d, f = path.Split("myfile.css")
fmt.Printf("path.Split(%q) = %q %q\n", "myfile.css", d, f)

d, f = path.Split("")
fmt.Printf("path.Split(%q) = %q %q\n", "", d, f)

The output:

path.Split("static/myfile.css") = "static/" "myfile.css"
path.Split("myfile.css") = "" "myfile.css"
path.Split("") = "" ""

@rsc
Copy link
Contributor

rsc commented Jan 29, 2018

It seems like the simplest fix is to swap the second and third line.

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 29, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 29, 2018
@awoodbeck
Copy link
Contributor

awoodbeck commented Jan 30, 2018

This simple change exposed a limitation in example output testing. The example works if the fmt.Println(path.Split("")) line is either the first or last line in the example, but not the middle line. Its single whitespace character and new line are stripped before comparison if it's on either end of the output, but not if it's in the middle (see the example output below).

Adding a whitespace character in the example output doesn't help either because each line is trimmed before being joined with a new line character as best I can tell.

--- FAIL: ExampleSplit (0.00s)                            
got:                                                      
"static/ myfile.css\n \n myfile.css"                      
want:
"static/ myfile.css\n\n myfile.css"

@josharian
Copy link
Contributor Author

Possibly related: #6416

@awoodbeck
Copy link
Contributor

@josharian I'm making certain to preserve the trailing space on the second line in the example (i.e., omit gofmt), two spaces after the // since the first space is always removed when building the ast. But I don't think it will matter either way since all trailing spaces are stripped when parsing the example output.

The unique aspect about this example is the single whitespace character in the middle line as opposed to a blank line. Stdout will always display the whitespace character but I cannot see a way to preserve the whitespace character in the example output.

@awoodbeck
Copy link
Contributor

Here's a proposed compromise:

Code:

fmt.Println(path.Split("static/myfile.css"))
fmt.Println(path.Split("myfile.css"))
fmt.Printf("%q", fmt.Sprintln(path.Split("")))

Output:

static/ myfile.css
myfile.css
" \n"

This passes but it may convolute the last example.

@josharian
Copy link
Contributor Author

I dunno. It'd be nice for all three to be constructed similarly. I tried this, but it's even worse:

package main

import (
	"fmt"
	"path"
)

func main() {
	for _, s := range []string{"static/myfile.css", "myfile.css", ""} {
		file, dir := path.Split(s)
		fmt.Printf("path.Split(%q): file=%q dir=%q\n", s, file, dir)
	}
}

I can't say I have any better ideas at this point.

@awoodbeck
Copy link
Contributor

Here's another suggestion that similarly constructs all 3 examples and passes.

out := func(args ...interface{}) {
	fmt.Printf("%q\n", fmt.Sprintln(args...))
}

out(path.Split("static/myfile.css"))
out(path.Split("myfile.css"))
out(path.Split(""))

Output:

"static/ myfile.css\n"
" myfile.css\n"
" \n"

@awoodbeck
Copy link
Contributor

Any opinion on my last example? I will submit it as a CL if it's received better than the current example.

@josharian
Copy link
Contributor Author

On m phone, but: it is nice for examples to be copy-paste friendly. In particular, having a line that reads:

file, sir := path.Split(x)

with useful mnemonic variable names seems valuable. And then we could avoid nested fmt calls and a code golf flavor.

@irbekrm
Copy link
Contributor

irbekrm commented Aug 22, 2019

Hey folks, I just did a Go contributors' workshop at London GopherCon and was looking at this as potentially my first issue to try to contribute.
Has there been any further decision about this issue?

@mvdan
Copy link
Member

mvdan commented Aug 22, 2019

@irbekrm I don't think there was a consensus so far; I'd say pick a solution you like (or your own solution), and we can always discuss it on the CL.

@irbekrm
Copy link
Contributor

irbekrm commented Aug 22, 2019

Thanks @mvdan

@gopherbot
Copy link

Change https://golang.org/cl/191397 mentions this issue: path: change the output format of ExampleSplit function

t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
At the moment the last output line of ExampleSplit- two empty strings- are being
trimmed from the output.  I have formatted the output of the function to avoid
whitespace trimming and show empty strings more clearly.

Fixes golang#23542

Change-Id: Ic2a4d98cfa06db1466c6c6d98099542df9e7c88b
Reviewed-on: https://go-review.googlesource.com/c/go/+/191397
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
@golang golang locked and limited conversation to collaborators Aug 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants