code review 105030046: time: change formatting of microseconds duration to SI ...

time: change formatting of microseconds duration to SI modifier
'u' is not micro, µ (U+00B5) is.

[r, 2014-06-11 15:14:58]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-06-11 17:16:01]

This seems like a borderline violation of the spirit of the Go 1 API
contract. I'm sure somebody, somewhere is depending on "us".



On Wed, Jun 11, 2014 at 8:14 AM, <r@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> time: change formatting of microseconds duration to SI modifier
> 'u' is not micro, µ (U+00B5) is.
>
> Please review this at https://codereview.appspot.com/105030046/
>
> Affected files (+13, -13 lines):
>   M src/pkg/time/example_test.go
>   M src/pkg/time/time.go
>   M src/pkg/time/time_test.go
>
>
> Index: src/pkg/time/example_test.go
> ===================================================================
> --- a/src/pkg/time/example_test.go
> +++ b/src/pkg/time/example_test.go
> @@ -122,7 +122,7 @@
>         }
>         // Output:
>         // t.Round(   1ns) = 12:15:30.918273645
> -       // t.Round(   1us) = 12:15:30.918274
> +       // t.Round(   1µs) = 12:15:30.918274
>         // t.Round(   1ms) = 12:15:30.918
>         // t.Round(    1s) = 12:15:31
>         // t.Round(    2s) = 12:15:30
> @@ -150,7 +150,7 @@
>
>         // Output:
>         // t.Truncate(   1ns) = 12:15:30.918273645
> -       // t.Truncate(   1us) = 12:15:30.918273
> +       // t.Truncate(   1µs) = 12:15:30.918273
>         // t.Truncate(   1ms) = 12:15:30.918
>         // t.Truncate(    1s) = 12:15:30
>         // t.Truncate(    2s) = 12:15:30
> Index: src/pkg/time/time.go
> ===================================================================
> --- a/src/pkg/time/time.go
> +++ b/src/pkg/time/time.go
> @@ -475,29 +475,29 @@
>         if u < uint64(Second) {
>                 // Special case: if duration is smaller than a second,
>                 // use smaller units, like 1.2ms
> -               var (
> -                       prec int
> -                       unit byte
> -               )
> +               var prec int
> +               w--
> +               buf[w] = 's'
> +               w--
>                 switch {
>                 case u == 0:
>                         return "0"
>                 case u < uint64(Microsecond):
>                         // print nanoseconds
>                         prec = 0
> -                       unit = 'n'
> +                       buf[w] = 'n'
>                 case u < uint64(Millisecond):
>                         // print microseconds
>                         prec = 3
> -                       unit = 'u'
> +                       // U+00B5 'µ' micro sign == 0xC2 0xB5
> +                       buf[w] = 0xB5 // "µ"[1]
> +                       w--
> +                       buf[w] = 0xC2 // "µ"[0]
>                 default:
>                         // print milliseconds
>                         prec = 6
> -                       unit = 'm'
> +                       buf[w] = 'm'
>                 }
> -               w -= 2
> -               buf[w] = unit
> -               buf[w+1] = 's'
>                 w, u = fmtFrac(buf[:w], u, prec)
>                 w = fmtInt(buf[:w], u)
>         } else {
> Index: src/pkg/time/time_test.go
> ===================================================================
> --- a/src/pkg/time/time_test.go
> +++ b/src/pkg/time/time_test.go
> @@ -535,7 +535,7 @@
>  }{
>         {"0", 0},
>         {"1ns", 1 * Nanosecond},
> -       {"1.1us", 1100 * Nanosecond},
> +       {"1.1µs", 1100 * Nanosecond},
>         {"2.2ms", 2200 * Microsecond},
>         {"3.3s", 3300 * Millisecond},
>         {"4m5s", 4*Minute + 5*Second},
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[r, 2014-06-11 20:33:00]

Yes, it is borderline. We need to decide whether this is OK to fix. I
stress the word fix, since the current notation is a bug. Although I'm
OK with accepting 'u' in the parser, since it's a convenience, it's
simply incorrect to use it as the output format. We should not be
afraid of non-ASCII and this should never have been accepted. Mea
culpa for not catching it earlier.

The correct prefix for micro is U+00B5, µ. There's a (Latin-1 plane!)
code point for exactly this purpose.

To me, the current situation is as wrong as using numerals as
prepositions and letters ("I am 2 c00l 4 that").

-rob

[gri, 2014-06-11 20:43:44]

LGTM

Not using µ looks like a bug to me. That's what Unicode is for.

[bradfitz, 2014-06-12 03:05:51]

LGTM

We can see if anybody complains during the cycle.  Add it to go1.4.txt at
least so we can reconsider/document it later.



On Wed, Jun 11, 2014 at 1:32 PM, Rob Pike <r@golang.org> wrote:

> Yes, it is borderline. We need to decide whether this is OK to fix. I
> stress the word fix, since the current notation is a bug. Although I'm
> OK with accepting 'u' in the parser, since it's a convenience, it's
> simply incorrect to use it as the output format. We should not be
> afraid of non-ASCII and this should never have been accepted. Mea
> culpa for not catching it earlier.
>
> The correct prefix for micro is U+00B5, µ. There's a (Latin-1 plane!)
> code point for exactly this purpose.
>
> To me, the current situation is as wrong as using numerals as
> prepositions and letters ("I am 2 c00l 4 that").
>
> -rob
>

[r, 2014-06-13 00:01:17]

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

time: change formatting of microseconds duration to SI modifier
'u' is not micro, µ (U+00B5) is.

LGTM=gri, bradfitz
R=golang-codereviews, bradfitz, gri
CC=golang-codereviews
https://codereview.appspot.com/105030046

[dsymonds, 2014-06-17 06:19:34]

This feels like a mistake. "u" is, you're right, technically incorrect, but I
agree with Brad that this feels like it violates the spirit of backward
compatibility. Moreso there are parsers for the same format (not "Go's
time.Duration format", but the general "NNu" form) that will now choke and
people will have to replace 'µ' with 'u' in their output to keep that working.
There are also plenty of places where ASCII is the only acceptable form, and
this breaks those too.

This feels like cleverness of style overriding pragmatism.

[r, 2014-06-17 14:11:20]

I will roll it back if it breaks too many things but I don't expect it to.

-rob


On Mon, Jun 16, 2014 at 11:19 PM,  <dsymonds@golang.org> wrote:
> This feels like a mistake. "u" is, you're right, technically incorrect,
> but I agree with Brad that this feels like it violates the spirit of
> backward compatibility. Moreso there are parsers for the same format
> (not "Go's time.Duration format", but the general "NNu" form) that will
> now choke and people will have to replace 'µ' with 'u' in their output
> to keep that working. There are also plenty of places where ASCII is the
> only acceptable form, and this breaks those too.
>
> This feels like cleverness of style overriding pragmatism.
>
> https://codereview.appspot.com/105030046/

[rsc, 2014-06-17 14:46:09]

LGTM

I wrote this code, and I support this change.

I chose u because I thought it was a standard mangling to ASCII, in the same way
that people write TeX and not τεχ.

When Rob mentioned this change to me, I made the same arguments that David and
Brad did, but then I went and looked around the internet for a while and I could
find very little support for writing u instead of µ. Yes, people do it, but it's
not nearly as widespread as I thought. (Maybe it is more common inside Google;
that would explain our collective confusion.) Of note is that the seemingly
widely used udunits program ("a C-based package for the programatic handling of
units of physical quantities") started with u but switched to µ (only) as well.
I also looked at the various standards documents for the SI prefixes, and there
is no mention there of a standard 7-bit ASCII representation. The Wikipedia
microsecond page never mentions 'us', and the microgram page says "When the
Greek lowercase “µ” (Mu) in the symbol µg is typographically unavailable, it is
occasionally—although not properly—replaced by the Latin lowercase “u”."

I also think there are likely to be very few parsers of the Go time.Duration
format other than Go's own, which already handles the µ form. If you were going
to interoperate with another language it would make far more sense to use a
fixed unit instead of the variable unit that Duration.String produces.

Like Rob said, if this breaks things, we can roll it back, but I believe this is
a bug fix plain and simple: µs was spelled wrong, and now it is spelled
correctly.

[rog, 2014-10-14 09:51:06]

FWIW this has just broken some of our tests.

But they're easy to fix.


On 17 June 2014 15:46,  <rsc@golang.org> wrote:
> LGTM
>
> I wrote this code, and I support this change.
>
> I chose u because I thought it was a standard mangling to ASCII, in the
> same way that people write TeX and not τεχ.
>
> When Rob mentioned this change to me, I made the same arguments that
> David and Brad did, but then I went and looked around the internet for a
> while and I could find very little support for writing u instead of µ.
> Yes, people do it, but it's not nearly as widespread as I thought.
> (Maybe it is more common inside Google; that would explain our
> collective confusion.) Of note is that the seemingly widely used udunits
> program ("a C-based package for the programatic handling of units of
> physical quantities") started with u but switched to µ (only) as well. I
> also looked at the various standards documents for the SI prefixes, and
> there is no mention there of a standard 7-bit ASCII representation. The
> Wikipedia microsecond page never mentions 'us', and the microgram page
> says "When the Greek lowercase “µ” (Mu) in the symbol µg is
> typographically unavailable, it is occasionally—although not
> properly—replaced by the Latin lowercase “u”."
>
> I also think there are likely to be very few parsers of the Go
> time.Duration format other than Go's own, which already handles the µ
> form. If you were going to interoperate with another language it would
> make far more sense to use a fixed unit instead of the variable unit
> that Duration.String produces.
>
> Like Rob said, if this breaks things, we can roll it back, but I believe
> this is a bug fix plain and simple: µs was spelled wrong, and now it is
> spelled correctly.
>
>
> https://codereview.appspot.com/105030046/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[rsc, 2014-10-14 14:19:17]

On Tue, Oct 14, 2014 at 5:51 AM, roger peppe <rogpeppe@gmail.com> wrote:

> FWIW this has just broken some of our tests.
>

How? Were you not parsing the output with Go? Were you expecting a golden
string?

Russ

[rog, 2014-10-15 11:44:05]

On 2014/10/14 14:19:17, rsc wrote:
> On Tue, Oct 14, 2014 at 5:51 AM, roger peppe <mailto:rogpeppe@gmail.com>
wrote:
> 
> > FWIW this has just broken some of our tests.
> >
> 
> How? Were you not parsing the output with Go? Were you expecting a golden
> string?
> 
> Russ

The test was testing log output. Part of the output included a request duration
matched against the regular expression [0-9.ums]+.