chore: make quoted failure messages more readable

[stevenh]

Summary

Use backtick quotes method in place of %q where possible so that output is more readable for simple strings with quotes.

Changes

• Use quote(str) instead of %q formatter
• Run go fmt ./... to correct spacing in comments.

Motivation

Makes it easier for a user to read the output without all the escaped double quotes.

Turns:

  expected: "my thing with \"quoted parts\"`

into:

  expected: `my thing with "quoted parts"`

[stevenh]

It's been a while so just checking in if we could get this over the line @brackendawson @MovieStoreGuy ?

[stevenh]

To me this is an acceptable change because it follows principles in the standard library.

Two concerns:

• This is a tiny minority of the %q format strings in even the module, should be consistent.
• This will default to backquote except where is is not possible, even when there's no escaping in the %q version. I would maybe be willing to accept this.

Thanks for the feedback @brackendawson.

On the first point I specifically picked the entries which were expected or actual and natural strings vs others such as lists etc to avoid any unintended side effects such as quoting a diff output as an example.

On the second point we could change the function to something like the following to prefer double quotes.

// quote returns the most readable (unescaped) version of s quoted.
// It prefers double quotes over backticks.
func quote(s string) string {
	quoted := strconv.Quote(s)
	if quoted == `"`+s+`"` {
		// No escaping was necessary.
		return quoted
	}

	if strconv.CanBackquote(s) {
		// Backticks can be used without escaping.
		return "`" + s + "`"
	}

	// All cases of quoting would require escaping.
	return quoted
}

While looking at this again I noticed we could also just change %q to %#q to prefer backticks if possible otherwise double, eliminating the need for custom function. I think this might be the better way to go, as that would be much easier to leverage in other locations where appropriate.

Thoughts?

[brackendawson]

I beleive we're only using %q to intentionally quote strings, so it should be safe to use what we come up with here in all instances? When reviewing I'll need to audit each one to be sure. If we do want to go with the "alternate" format for quoted strings, then %#q is far preferable to any custom function.

The character on my left shoulder says please just be normal, the character on my right shoulder says the alternate format has to escape in fewer cases. I'm not sure which is the angel and which is the devil.

I'd like to see some more people's opinions. Maybe worth asking on #testify-dev.

[dolmen]

@stevenh This PR is 2 years old. Please rebase first.

[stevenh]

Rebased on latest version, just some additional test data updated and tweek to handle the restructured buildErrorChainString.

[brackendawson]

This is still a minority of testify's format strings with %q. If we change the quote type it should be all of them or we are just making testify less consistent.

Though I think:

"\"this\""

is actually more understood than:

`"this"`

[ccoVeille]

I like the PR idea. It would bring clarity.

I feel like only people who ~~suffered~~ faced years of JSON fun, can read the quote format without twitching.

The fact is used strconv.CanBackquote makes it robust.

But I agree if we go into this direction, I would prefer a PR that updates everything everywhere.