andygrunwald / go-gerrit

Go client/library for Gerrit Code Review
https://godoc.org/github.com/andygrunwald/go-gerrit
MIT License
96 stars 40 forks source link

Fix type of EventInfo.PatchSet.Number #37

Closed opalmer closed 7 years ago

opalmer commented 7 years ago

Depending on the version of the events-log plugin and the version of Gerrit you can end up in a situation where some values of EventInfo.PatchSet.Number are a string and other are an integer. This change works around the problem by replacing the Number field with a custom type that can handle either strings or integers.

Note, this does not change the underlying type of the Number field.

codecov-io commented 7 years ago

Codecov Report

Merging #37 into master will increase coverage by 0.6%. The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #37     +/-   ##
=========================================
+ Coverage   17.45%   18.06%   +0.6%     
=========================================
  Files          20       21      +1     
  Lines        1724     1738     +14     
=========================================
+ Hits          301      314     +13     
- Misses       1393     1394      +1     
  Partials       30       30
Impacted Files Coverage Δ
events.go 69.23% <ø> (ø) :arrow_up:
types.go 92.85% <92.85%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 73dc271...16138b0. Read the comment docs.

andygrunwald commented 7 years ago

LGTM

opalmer commented 7 years ago

Thanks! I was really hoping there was a better way of handling this problem but I couldn't think of one. On the bright side, it's isolated to the plugin it seems and at least we'll have a way to deal with this if Gerrit itself has this kind of problem in the future.

On a side note, any objections to me cutting a new release?

andygrunwald commented 7 years ago

I often follow the principle: Make it work, make it beautiful, make it fast. Your solution looks fine in my opinion and is simple to understand. This is one of the main points.

On a side note, any objections to me cutting a new release?

Go for it. Just prepare the release notes to show what has changed.

opalmer commented 7 years ago

Go for it. Just prepare the release notes to show what has changed.

Will do, working on that now.