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.

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

Fix overlapping struct fields & last golint errors #332

Merged
merged 1 commit into from
Oct 12, 2021

Conversation

theckman
Copy link
Collaborator

There have been a few issues filed over the past few years, relative to
unexpected or unreliable behavior in the client, due to overlapping struct
fields because of an embedded type or conflicting JSON field tags. This could
make it appear like some fields are unset, because encoding/json has put the
value in the overlapped field and not the one the programmer is accessing.
Unfortunately, fixing these issues are technically a breaking API change and so
we waited to completely fix them until now.

In the spirit of making the struct fields consistently named, this also updates
the HttpStatus field in the EventResponse struct to be HTTPStatus. This is
purely cosmetic, but feels appropriate to do now since the removal if the Id
field makes it the last one.

Fixes #218
Fixes #316
Fixes #268

There have been a few issues filed over the past few years, relative to
unexpected or unreliable behavior in the client, due to overlapping struct
fields because of an embedded type or conflicting JSON field tags. This could
make it appear like some fields are unset, because `encoding/json` has put the
value in the overlapped field and not the one the programmer is accessing.
Unfortunately, fixing these issues are technically a breaking API change and so
we waited to completely fix them until now.

In the spirit of making the struct fields consistently named, this also updates
the `HttpStatus` field in the `EventResponse` struct to be `HTTPStatus`. This is
purely cosmetic, but feels appropriate to do now since the removal if the `Id`
field makes it the last one.

Fixes #218
Fixes #316
Fixes #268
@theckman theckman added this to the v1.5.0 milestone May 16, 2021
@theckman theckman added the breaking change This PR contains a breaking change label May 16, 2021
Copy link
Contributor

@stmcallister stmcallister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for sure needed. Thanks for cleaning these up! 👍 👏 🎉

@theckman theckman merged commit 26bf695 into master Oct 12, 2021
@theckman theckman deleted the fix_overlapping_fields_and_golint branch October 12, 2021 05:47
theckman added a commit that referenced this pull request Oct 12, 2021
In between raising #332 and merging it, there were changes to the master branch
that resulted in the tests in this PR breaking after the merge.

In hindsight, this branch should have been rebased against master before merging
the PR.
theckman added a commit that referenced this pull request Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This PR contains a breaking change
Projects
None yet
2 participants