code review 7387044: strconv, fmt: clarify behavior of CanBackquote and "%#q".

strconv, fmt: clarify behavior of CanBackquote and "%#q".
Fixes issue 4858.

[minux1, 2013-02-20 17:09:26]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-02-20 18:20:25]

https://codereview.appspot.com/7387044/diff/11001/src/pkg/fmt/doc.go
File src/pkg/fmt/doc.go (right):

https://codereview.appspot.com/7387044/diff/11001/src/pkg/fmt/doc.go#newcode77
src/pkg/fmt/doc.go:77: for %q, print a raw (backquoted) string if
strconv.CanBackquote() returns true, and the result will always be a single
line;
drop parens and wrap this.  Also I think you can s/,.*//, since CanBackquote now
says that.  It's redundant.

https://codereview.appspot.com/7387044/diff/11001/src/pkg/strconv/quote.go
File src/pkg/strconv/quote.go (right):

https://codereview.appspot.com/7387044/diff/11001/src/pkg/strconv/quote.go#ne...
src/pkg/strconv/quote.go:144: // Although '\n' is fine within backquotes,
CanBackquote
I guess this was always a weird predicate, but now it's explicitly so.

In retrospect, I would've preferred CanBackquote (include \n) and
CanBackquoteSingleLine.

*shrug*

[minux1, 2013-02-20 18:49:12]

PTAL.

On 2013/02/20 18:20:25, bradfitz wrote:
> https://codereview.appspot.com/7387044/diff/11001/src/pkg/fmt/doc.go#newcode77
> src/pkg/fmt/doc.go:77: for %q, print a raw (backquoted) string if
> strconv.CanBackquote() returns true, and the result will always be a single
> line;
> drop parens and wrap this.  Also I think you can s/,.*//, since CanBackquote
now
> says that.  It's redundant.
Done.

>
https://codereview.appspot.com/7387044/diff/11001/src/pkg/strconv/quote.go#ne...
> src/pkg/strconv/quote.go:144: // Although '\n' is fine within backquotes,
> CanBackquote
> I guess this was always a weird predicate, but now it's explicitly so.
> 
> In retrospect, I would've preferred CanBackquote (include \n) and
> CanBackquoteSingleLine.
Yeah, i was also puzzled by the behavior of CanBackquote and that's why
i asked on the issue tracker.
Fortunately, implementing a real CanBackquote won't be difficult and at
least we now give a warning in the docs.

[r, 2013-02-20 20:24:34]

https://codereview.appspot.com/7387044/diff/14004/src/pkg/strconv/quote.go
File src/pkg/strconv/quote.go (right):

https://codereview.appspot.com/7387044/diff/14004/src/pkg/strconv/quote.go#ne...
src/pkg/strconv/quote.go:145: // will return false for any multiline strings so
that
it's not just multiline; `abc
` is arguably one line.
how about (plus it's shorter':

will return false if s contains a newline.

https://codereview.appspot.com/7387044/diff/14004/src/pkg/strconv/quote.go#ne...
src/pkg/strconv/quote.go:146: // `s` will always be a single line when printed
out.
this seems overexplaining to me. i'd drop bit, but you can leave it in if you
think it adds information. (i don't)

[rsc, 2013-02-20 20:49:40]

https://codereview.appspot.com/7387044/diff/14004/src/pkg/strconv/quote.go
File src/pkg/strconv/quote.go (right):

https://codereview.appspot.com/7387044/diff/14004/src/pkg/strconv/quote.go#ne...
src/pkg/strconv/quote.go:145: // will return false for any multiline strings so
that
On 2013/02/20 20:24:34, r wrote:
> it's not just multiline; `abc
> ` is arguably one line.
> how about (plus it's shorter':
> 
> will return false if s contains a newline.

It's not just multiline. It returns false for all non-tab control characters.

[minux1, 2013-02-23 10:45:15]

PTAL.

[r, 2013-02-23 16:55:41]

https://codereview.appspot.com/7387044/diff/10003/src/pkg/strconv/quote.go
File src/pkg/strconv/quote.go (right):

https://codereview.appspot.com/7387044/diff/10003/src/pkg/strconv/quote.go#ne...
src/pkg/strconv/quote.go:145: // '\n') is fine within backquotes, CanBackquote
s/is/are/

https://codereview.appspot.com/7387044/diff/10003/src/pkg/strconv/quote.go#ne...
src/pkg/strconv/quote.go:146: // will return false if they appear in s.
how about this, which helps clarify things better?

CanBackquote reports whether the string s can be represented unchanged
as a single-line backquoted string without control characters other than
space and tab.

[minux1, 2013-02-23 17:00:13]

PTAL.

On Sun, Feb 24, 2013 at 12:55 AM, <r@golang.org> wrote:

> https://codereview.appspot.**com/7387044/diff/10003/src/**
>
pkg/strconv/quote.go#**newcode146<https://codereview.appspot.com/7387044/diff/10003/src/pkg/strconv/quote.go#newcode146>
> src/pkg/strconv/quote.go:146: // will return false if they appear in s.
> how about this, which helps clarify things better?
>
> CanBackquote reports whether the string s can be represented unchanged
> as a single-line backquoted string without control characters other than
> space and tab.
>
Great. This clarifies the rationale behind the behavior at the same.

[minux1, 2013-02-23 17:01:11]

On Sun, Feb 24, 2013 at 12:59 AM, minux <minux.ma@gmail.com> wrote:

> PTAL.
>
> On Sun, Feb 24, 2013 at 12:55 AM, <r@golang.org> wrote:
>
>> https://codereview.appspot.**com/7387044/diff/10003/src/**
>>
pkg/strconv/quote.go#**newcode146<https://codereview.appspot.com/7387044/diff/10003/src/pkg/strconv/quote.go#newcode146>
>> src/pkg/strconv/quote.go:146: // will return false if they appear in s.
>> how about this, which helps clarify things better?
>>
>> CanBackquote reports whether the string s can be represented unchanged
>> as a single-line backquoted string without control characters other than
>> space and tab.
>>
> Great. This clarifies the rationale behind the behavior at the same.
>
... at the same time. (premature send)

[r, 2013-02-23 18:14:32]

LGTM but check with rsc too

[rsc, 2013-02-25 21:52:22]

LGTM

I don't know whether space is a control character. I would have guessed not.

[r, 2013-02-25 21:53:05]

whether it is or not, the meaning seems clear.

[minux1, 2013-02-25 22:34:17]

*** Submitted as https://code.google.com/p/go/source/detail?r=8e4103edc064 ***

strconv, fmt: clarify behavior of CanBackquote and "%#q".
Fixes issue 4858.

R=golang-dev, bradfitz, r, rsc
CC=golang-dev
https://codereview.appspot.com/7387044