Go-github: Dubious implementation of Label.String method.

[dmitshur]

This came up during PR #593. /cc @bradfitz @willnorris @gmlewis

The implementation of Label.String is currently unusual and has no comment (or commit message via git blame) that explains why it's different from the 73 other String method implementations that simply do return Stringify(v).

There are 2 aspects to it:

1. The code return fmt.Sprint(*l.Name) can be simplified to return *l.Name without change of behavior. To a casual reader of the code, it's unclear why that hasn't been done.

2. The Label.String method prints the name of label, and not any of the other fields. Without a comment, it's unclear if that deviation of behavior is intentional or not.

I've dug up the PR that added the code (in August 2013), and the discussion in that PR actually explains both things:

github.com/google/go-github/pull/32

It looks like the reason return fmt.Sprint(*l.Name) is used instead of simpler return *l.Name is because the PR originally used fmt.Sprintf, and @willnorris correctly suggested that fmt.Sprint would be better to use than fmt.Sprintf, since there isn't a format string (and if there was, it'd be simply "%s"). It looks like it was an oversight that return *l.Name wasn't suggested.

As far as not using Stringify in String, it was intentional in the context of that PR, see https://github.com/google/go-github/pull/32#commitcomment-3789370:

just curious, why the special String() func here? If someone only wants the label name, they could always just call label.Name.

I am using go-github to build some one of report and displaying the labels without iterating is handy

fmt.Println(issue.Number, issue.State, issue.Labels, issue.Title)

Here it is the kind of useful display :

7 closed [enhancement back-end] define and implement the multisite strategy

ah, got it. Works for me.

We should either document that it's intentional by adding a comment, or just change the behavior to be consistent with all other String methods, and use Stringify.

The current unusual behavior of Label.String is not documented, so I don't think anyone else except that one person can actually rely on it. If someone wants to rely on it, it's better to implement it themselves instead.

[gmlewis]

I like changing its behavior to be consistent and use Stringify.

[willnorris]

+1, okay by me.

[willnorris]

/cc @yml as FYI