Add JSON tag for ErrorResponse #2641
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2641 +/- ##
=======================================
Coverage 98.05% 98.05%
=======================================
Files 130 130
Lines 11242 11244 +2
=======================================
+ Hits 11023 11025 +2
Misses 150 150
Partials 69 69
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Fixes: #2640. |
gmlewis
added
waiting for reply
DO NOT MERGE
github/github.go
Outdated
| Response *http.Response // HTTP response that caused this error | ||
| Message string `json:"message"` // error message | ||
| Errors []Error `json:"errors"` // more detail on individual errors | ||
| Response *http.Response `json:"omitempty"` // HTTP response that caused this error |
There was a problem hiding this comment.
I've never seen a bare "omitempty" before, so can we please name it as "response"?
| Response *http.Response `json:"omitempty"` // HTTP response that caused this error | |
| Response *http.Response `json:"response,omitempty"` // HTTP response that caused this error |
There was a problem hiding this comment.
I'm fine adding the name of the field. But I wonder in this case if it might needs to be Response to maintain compatibility with the current code.
There was a problem hiding this comment.
You are saying it should be "Response" with a capital-R ?
When you say "compatibility with the current code", do you mean the "go-github" repo?
If so, I'm not sure why we would need to capitalize the R, and I would think the lower-case version should be fine.
Alternatively, another approach might be to simply use json:"-" and always leave it out... but I'll let you decide if that is the best option for compatibility with your mocking package.
There was a problem hiding this comment.
Hi Glenn, I actually liked the idea of going with json:"-". Let me test a few things and I will come back to you.
Thanks for the swift responses!
There was a problem hiding this comment.
Hi @gmlewis , it seems the json:"-" should work just fine. I will update the PR soon.
migueleliasweb
force-pushed
the
add-missing-json-tag-errorresponse
branch
from
c93ce7b
to
2ea160d
Compare
migueleliasweb
force-pushed
the
add-missing-json-tag-errorresponse
branch
from
2ea160d
to
bafa709
Compare
There was a problem hiding this comment.
Thank you, @migueleliasweb !
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
|
It's always a pleasure, @gmlewis 😃 . Is there someone I could ping here to get this going? |
|
@Parker77 - do you have time for an LGTM+Approval, please? |
|
Thank you, @Parker77! |
|
Btw, merging this fixed the default error formatting issue on |
Fixes: #2640.