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

Make TransactionSearchResponse.Data.Fields a json.RawMessage #23

Closed
wants to merge 1 commit into from

Conversation

kylec1
Copy link

@kylec1 kylec1 commented May 19, 2020

The present keys and value types change depending on the transaction,
and using json.RawMessage allows it to be mapped to an appropriate
struct once they type is known rather than needing to fish the values
out of a messy map of interface{}.

This is certainly a breaking change for any consumers of Fields, but
one that should be easy to identify at compile time, and can be fixed
with a simple json.Marshal call.

The present keys and value types change depending on the transaction,
and using json.RawMessage allows it to be mapped to an appropriate
struct once they type is known rather than needing to fish the values
out of a messy map of interface{}.

This is certainly a breaking change for any consumers of Fields, but
one that should be easy to identify at compile time, and can be fixed
with a simple json.Marshal call.
@github-actions
Copy link

Stale pull request message

@peterebden
Copy link
Member

Sorry, this one slipped by me at the time... Let me check internally about how we use this and what it'd mean.

TBH I wonder if really there should be concrete types for each SearchResponse struct...

@geonaut
Copy link

geonaut commented Jul 20, 2020

+1 on adding the concrete types. This is the way that the upstream does it, so we might end up with conflicts. Can see why you've taken this approach though.

@peterebden
Copy link
Member

I think that's the direction we should take. If we're going to break compatibility with these messages (something I am not against in general, we use this in a relatively limited set of places internally and I'm not sure there are many other people using it) I think I'd rather do it "properly" to get to the better possible end state.

Thanks for the pull request anyway!

@peterebden peterebden closed this Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants