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

Comments

@josharian
Copy link
Contributor

@josharian 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.)

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan 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.

@awoodbeck

This comment has been minimized.

Copy link
Contributor

@awoodbeck 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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 29, 2018

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

@rsc rsc added the NeedsFix label Jan 29, 2018
@awoodbeck

This comment has been minimized.

Copy link
Contributor

@awoodbeck 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

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Jan 30, 2018

Possibly related: #6416

@awoodbeck

This comment has been minimized.

Copy link
Contributor

@awoodbeck awoodbeck commented Jan 30, 2018

@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

This comment has been minimized.

Copy link
Contributor

@awoodbeck awoodbeck commented Jan 30, 2018

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

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Feb 1, 2018

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

This comment has been minimized.

Copy link
Contributor

@awoodbeck awoodbeck commented Feb 5, 2018

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

This comment has been minimized.

Copy link
Contributor

@awoodbeck awoodbeck commented Feb 12, 2018

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

@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Feb 12, 2018

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

This comment has been minimized.

Copy link
Contributor

@irbekrm 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

This comment has been minimized.

Copy link
Member

@mvdan 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

This comment has been minimized.

Copy link
Contributor

@irbekrm irbekrm commented Aug 22, 2019

Thanks @mvdan

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 22, 2019

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

@gopherbot gopherbot closed this in 54a9c16 Sep 1, 2019
t4n6a1ka added 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.