Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make _account_id optional in AccountInfo #19

Merged
merged 4 commits into from
Oct 13, 2016
Merged

Make _account_id optional in AccountInfo #19

merged 4 commits into from
Oct 13, 2016

Conversation

opalmer
Copy link
Contributor

@opalmer opalmer commented Oct 12, 2016 •
edited
Loading

This should address #16.

@opalmer
_account_id should be omitted if empty (closes #16)
5b4aedb 
The AccountInfo struct is used in other structs, such
as ApprovalInfo, and _account_id may not always be
set in those cases. In these cases if the _account_id
field is not setup with 'omitempty' then the field will
be included anyway even if Gerrit does not return it.

Thanks to @perolausson for finding and explaining the
issue.
@opalmer
adding changelog
8a94d7f
@opalmer opalmer added the bug label Oct 12, 2016
@opalmer opalmer added this to the 0.1.1 milestone Oct 12, 2016
@opalmer opalmer self-assigned this Oct 12, 2016
@coveralls
Copy link

coveralls commented Oct 12, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 10.495% when pulling 8a94d7f on fix_account_id into bbeff44 on master.

@opalmer
Copy link
Contributor Author

opalmer commented Oct 12, 2016

@perolausson would you mind taking a look? I think this addresses what you brought up in #16 but I wanted to be sure before it's merged.

@perolausson
Copy link
Contributor

I'll try it out later this evening!

On 12 Oct 2016 16:36, "opalmer" notifications@github.com wrote:

@perolausson https://github.com/perolausson would you mind taking a
look? I think this addresses what you brought up in #16
#16 but I wanted to be
sure before it's merged.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AC2X-ORCyPpjqFHeI68xTxgYFZEG-6Ynks5qzP5xgaJpZM4KU5P8
.

@coveralls
Copy link

coveralls commented Oct 12, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 10.495% when pulling be7a4af on fix_account_id into 075ba1b on master.

@andygrunwald
Copy link
Owner

LGTM

@perolausson perolausson mentioned this pull request Oct 12, 2016
Closed
@opalmer opalmer merged commit 7e459ad into master Oct 13, 2016
@opalmer opalmer deleted the fix_account_id branch October 13, 2016 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@opalmer opalmer

Labels
bug
Projects
None yet
Milestone
0.1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@opalmer @coveralls @perolausson @andygrunwald