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

Add an option to include maybe_stream_resumption step #159

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kanes115
Copy link
Contributor

This PR let's user tell connection module that it wants to enable stream management with resume option. It is now not available without specifying all the steps in escalus_connection:start/2.

Now it would be enough to add {connection_steps, default_resume} to user spec in escalus_connection:start/2. This PR does not change the API, just extends it (with this option).

I add this as I needed it in this PR

If I miss the way I should specify that I want to use maybe_stream_resumption, let me know, please :)

@kanes115 kanes115 added the WIP label Dec 18, 2017
@erszcz
Copy link
Member

erszcz commented Jan 4, 2018

Nice idea!

However, I have a hunch it would be a bit misleading if connection_steps was reused in such a way, since the name suggests it's a list/group of things. Still, there's a stream_management (now boolean) option allowed in the user spec and handled by maybe_stream_management and maybe_stream_resumption in escalus_session.

Could you rework this patch to introduce a new resume_by_default value of stream_management option, so that it's picked up by the relevant functions in escalus_session?

@erszcz
Copy link
Member

erszcz commented Jan 4, 2018

Specifically, maybe_stream_management could have one more case clause for the new value of the option.

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

Successfully merging this pull request may close these issues.

None yet

2 participants