search

sonyxperiadev / gerrit-events

MIT License
47 stars 62 forks source link

Add support for ChangeKind #17

Closed vangdfang closed 10 years ago

vangdfang commented 10 years ago

Gerrit master extends the PatchsetCreated event to include the ChangeKind attribute, which exposes certain types of trivial changes.

rsandell commented 10 years ago

I'm thinking a bit on the same track as @rinrinne that an enum would be more appropriate than a string, and simpler to compare to. A bit similar thought as in #16

The question is though is how to handle when Gerrit adds more types, would it be enough to have an UNKNOWN type to return in those cases?

rsandell commented 10 years ago

Should we differentiate between null and UNKNOWN? I think we should; null indicating that the kind wasn't provided at all and UNKNOWN specifying that there is one but not one that is recognized. But I can be convinced otherwise ;)

vangdfang commented 10 years ago

Good point. I doubt there will be many unknowns (as in not recognized) in practice -- we already know that "NEW_CHANGE" is proposed as new (not yet written in Gerrit) change kind -- so no idea when we'll be seeing that. Otherwise, nulls are a bit obnoxious to deal with in Java, I think. It seems nicer to return an enum value (even if we had a unknown and not set enum value) in all cases, to avoid accidentally using a null pointer somewhere (a null enum might catch people off guard).

rsandell commented 10 years ago

Well at the same time there are null values everywhere in the code for attributes not yet implemented in the Gerrit version that we are talking to at the moment. So far null has been an indication the the data was not available in the stream. So for devs used to this module not returning null would be a different behavior from the rest of the code, particular in this case when the attribute isn't available in any released Gerrit version yet.

rsandell commented 10 years ago

Released in 2.1.0 and should be in Maven central shortly

vangdfang commented 10 years ago

Thanks @rsandell, updated https://github.com/jenkinsci/gerrit-trigger-plugin/pull/159 accordingly!