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

[RFC] Alternative approach to "related events". #240

Merged
merged 3 commits into from
Jun 28, 2016
Merged

Conversation

Lukasa
Copy link
Member

@Lukasa Lukasa commented Jun 14, 2016

This is an alternative approach to the "related events" API, as discussed in #234. Another approach is in #238, which should be read for the primary discussion.

This RFC differs from #238 by strictly enumerating the possible related events for each primary event and hanging those events directly off named properties. This API has the advantage that it's extremely explicit: code cannot accidentally misbehave anywhere near as easily as it can with #238. However, it has the disadvantage that it's substantially less extensible: if more related events pop up, it requires additional work to plumb them through in the right places.

Regardless, this is worth looking at. /cc @Kriechi @mhils @python-hyper/contributors

@Kriechi
Copy link
Member

Kriechi commented Jun 14, 2016

Thanks! I really like this approach and the neat API design. 👍

Maybe one question for a possible backward-compatible opt-in feature:
Instead of extending events with StreamEnded and PriorityUpdated, can we make it optional to suppress these events being fired individually, so that they are only attached to their "parent" event?
I think this could prevent possible double-processing of events.

Do you think a simple True/False would be sufficient instead of attaching a full StreamEnded event?

Let me tinker around with it in @mitmproxy to see how this all works out. But I think this is a good step into the right direction.

@Lukasa
Copy link
Member Author

Lukasa commented Jun 15, 2016

So your two suggestions are slightly different, let's tackle them in turn:

Instead of extending events with StreamEnded and PriorityUpdated, can we make it optional to suppress these events being fired individually, so that they are only attached to their "parent" event?
I think this could prevent possible double-processing of events.

We certainly could do this. I think it's worth considering, in part because it makes #228 dramatically simpler. We could essentially manage this transition like we've managed the transition for #154: add a flag on the connection object that affects this behaviour, and then remove the fallback for 3.0.0. That would then make it much easier to do the lazy iteration behaviour of #228.

What's not clear to me yet is whether this transforms the API in a manner that is altogether too expert-oriented. It's definitely a weird subtlety to have events that are only sometimes emitted at the top level, and the rest of the time need to be fished out of other events. We could keep the fallback around, but at that point we basically have two APIs that function entirely differently. Not ideal.

Do you think a simple True/False would be sufficient instead of attaching a full StreamEnded event?

The answer is probably, but it leads to a weird inconsistency with priority_updated. I'd rather both of those looked identical.

@Kriechi
Copy link
Member

Kriechi commented Jun 15, 2016

Ok, yes I think in that case it makes sense to keep StreamEnded as full event. This avoids confusion.

For the flag - I'm in favour of this. Maybe even considering setting it as default in 3.0.0 and for now just as opt-in.

👍

@Lukasa
Copy link
Member Author

Lukasa commented Jun 28, 2016

Ok, let's do this. I'll aim to push a release this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants