Labels handling hard-coded to assume only "default" labels are in use

[perolausson]

While verifying the change for issue #19 I realised that the current Labels handling seem hard coded to assume that only CodeReview and Verified exists as labels. I am wondering what would have happened if I had some other label defined for a project?

I am thinking about this code in changes.go:

// Labels entity contains the labels Verified & CodeReview, always corresponding to the current patch set.
type Labels struct {
    Verified   LabelInfo `json:"Verified,omitempty"`
    CodeReview LabelInfo `json:"Code-Review,omitempty"`
}

I actually like the way this is, but of course it would be a problem if I had some other label. I presume this means that Labels should be a map of LabelInfo instead, with keys being Verified, CodeReview, ...?

[opalmer]

Oddly enough, I came the same issue a few days ago because one of the instances of Gerrit that I manage uses several custom labels (which of course don't work with this struct).

I think using a map makes the most sense and would certainly be the route I would have taken if I had written the first implementation of Labels. But this is an existing struct so changing the implementation rather than say adding to it or changing how it's built, while not removing fields, would probably cause issues for consumers of go-gerrit.

I'm not certain what the best approach to fix and need to do some research but I'm certainly open to other suggestions. For now the ideas I have are:

• Create a new field on Labels to hold non-standard labels.
• Update the labels struct with functions to query non-standard labels
• Dynamically construct all fields on the struct (last I checked there are a few libraries out there that might be able to do this for us).

[andygrunwald]

At the point of implementation i didn`t know that custom labels are possible. This is, of course, a bug (or a missing feature, depends on the point of view).

I like the first two options. But i want to mention a third option:

1. Tag the current version
2. Do the breaking change by changing this to a map

I know this is not beautiful, but if we mark this as a new tag + write down the change in the ChangeLog this should be fine (i hope that all consumers are using vendoring). What do you think @opalmer and @perolausson?

[opalmer]

Yeah that's kind of the direction I was thinking actually. I can't really see any other better way in terms of implementation complexity or ease of use. Good thing we can tag releases now haha.

Once #22 lands we can tag 0.1.1.