Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Copy rather than move the fields to shuffle between a v1 and a v2 event. #402

Merged
merged 2 commits into from
Dec 1, 2015

Conversation

NegativeMjark
Copy link
Contributor

This should make all v1 APIs compatible with v2 clients. While still
allowing v1 clients to access the fields.

This makes the documentation easier since we can just document the v2
format and explain that some of the fields, in some of the APIs are
duplicated for backwards compatibility, rather than having to document
two separate event formats.

This should make all v1 APIs compatible with v2 clients. While still
allowing v1 clients to access the fields.

This makes the documentation easier since we can just document the v2
format and explain that some of the fields, in some of the APIs are
duplicated for backwards compatibility, rather than having to document
two separate event formats.
d["user_id"] = d.pop("sender", None)
d = format_event_for_client_v2(d)

d["user_id"] = d.get("sender", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this only set if it's present? I don't like the number of random nulls we already add to responses...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dunno. I don't think this changes the behaviour of the code from before. I don't think we should be seeing events without a sender either. I'm happy to switch this to a conditional if you prefer.

@illicitonion
Copy link
Contributor

LGTM modulo one question

NegativeMjark added a commit that referenced this pull request Dec 1, 2015
Copy rather than move the fields to shuffle between a v1 and a v2 event.
@NegativeMjark NegativeMjark merged commit d32db0b into develop Dec 1, 2015
@erikjohnston erikjohnston deleted the markjh/event_formatting branch January 12, 2016 16:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants