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.

Sign up for GitHub

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

Jump to bottom

Pass IDLE responses to caller. #186

Merged
merged 23 commits into from
Apr 20, 2021
Merged

Pass IDLE responses to caller. #186

merged 23 commits into from
Apr 20, 2021

Conversation

mordak
Copy link
Contributor

@mordak mordak commented Mar 28, 2021 •
edited by jonhoo
Loading

Picking up #130, but doing it a little differently. Implement passing IDLE responses to the caller via a callback function.

Also add an example, for #152.

This change is Reviewable

mordak added 2 commits March 28, 2021 15:27
@mordak
Implement a method to pass unilateral responses while IDLE.
529401a 
While IDLE, the server sends unilateral responses to notify the client of
changes to the mailbox as they happen. Instead of always exiting the IDLE
on any change, allow the caller to pass a callback function which receives
the messages and returns an action to either Continue IDLE or Stop and exit.

For clients wishing to use the previous behaviour, a callback_stop convenience
function is provided that terminates the IDLE on any change to the mailbox.
@mordak
Add IDLE example.
2874bfd
@codecov
Copy link

codecov bot commented Mar 28, 2021 •
edited
Loading

Codecov Report

Merging #186 (48db461) into master (4108917) will decrease coverage by 0.89%.
The diff coverage is 32.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
- Coverage   77.24%   76.35%   -0.90%     
==========================================
  Files          13       14       +1     
  Lines        1591     1607      +16     
==========================================
- Hits         1229     1227       -2     
- Misses        362      380      +18
Impacted Files Coverage Δ
src/error.rs 2.94% <0.00%> (ø)
src/extensions/idle.rs 0.00% <0.00%> (ø)
src/types/unsolicited_response.rs 31.42% <31.42%> (ø)
src/parse.rs 87.23% <43.75%> (+0.20%) ⬆️
src/types/mod.rs 41.93% <100.00%> (+1.93%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4108917...48db461. Read the comment docs.

@mordak
Copy link
Contributor Author

mordak commented Mar 28, 2021

Pipeline fail is a clippy lint redundant_slicing (on 1.52 beta) - I can fix that in another PR, or fix it here.

@jonhoo
Copy link
Owner

jonhoo commented Apr 1, 2021

Thank you for pushing on this! I'm hoping I'll have some time to review over the weekend — otherwise I'll get to it as soon as I can 🎉

jonhoo
jonhoo reviewed Apr 5, 2021
View reviewed changes
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Overall this looks great! I've left some comments inline, though nothing too major I don't think.

examples/idle.rs Show resolved Hide resolved
examples/idle.rs Show resolved Hide resolved
src/extensions/idle.rs Outdated Show resolved Hide resolved
src/extensions/idle.rs Outdated Show resolved Hide resolved
src/extensions/idle.rs Show resolved Hide resolved
src/types/unsolicited_response.rs Outdated Show resolved Hide resolved
src/types/unsolicited_response.rs Show resolved Hide resolved
src/types/unsolicited_response.rs Outdated Show resolved Hide resolved
src/types/unsolicited_response.rs Outdated Show resolved Hide resolved
examples/idle.rs Show resolved Hide resolved
jonhoo
jonhoo reviewed Apr 6, 2021
View reviewed changes
src/types/mod.rs Outdated Show resolved Hide resolved
@mordak
Improve documentation around unhandled responses.
9126d3c 
UnsolicitedResponse is not exhaustive, and open an issue if you find one
that isn't handled.
@mordak mordak mentioned this pull request Apr 8, 2021
Closed
jonhoo
jonhoo reviewed Apr 8, 2021
View reviewed changes
src/extensions/idle.rs Outdated Show resolved Hide resolved
jonhoo
jonhoo reviewed Apr 8, 2021
View reviewed changes
src/extensions/idle.rs Outdated Show resolved Hide resolved
jonhoo
jonhoo reviewed Apr 8, 2021
View reviewed changes
src/extensions/idle.rs Outdated Show resolved Hide resolved
jonhoo
jonhoo reviewed Apr 8, 2021
View reviewed changes
src/extensions/idle.rs Outdated Show resolved Hide resolved
@mordak mordak mentioned this pull request Apr 11, 2021
Closed
@jonhoo
Copy link
Owner

jonhoo commented Apr 15, 2021

Re our discussion about bool vs a rich type for continue vs break, it looks like maybe we'll get that type directly in the standard library! See rust-lang/rfcs#3058 and specifically the ControlFlow type.

@jonhoo
Copy link
Owner

jonhoo commented Apr 15, 2021

Better yet: rust-lang/rust#75744

@scottmcm
Copy link

I have high hopes that ControlFlow will stabilize soon™ -- the major thing blocking it is unwanted interconversions, which will be fixed as soon as RFC 3058 goes in on nightly. (It oughtn't need to wait for full Try stabilization.)

@mordak
Copy link
Contributor Author

mordak commented Apr 18, 2021

Re our discussion about bool vs a rich type for continue vs break, it looks like maybe we'll get that type directly in the standard library! See rust-lang/rfcs#3058 and specifically the ControlFlow type.

Oh, that will be nice!

@jonhoo
Copy link
Owner

jonhoo commented Apr 20, 2021

Let's maybe open a ticket to consider moving to ControlFlow if it lands in time for 3.0.0?

jonhoo
jonhoo approved these changes Apr 20, 2021
View reviewed changes
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

I think we're finally there! Thanks for the awesome work ❤️

@jonhoo jonhoo merged commit 382c025 into jonhoo:master Apr 20, 2021
This was referenced Apr 20, 2021
Closed
Closed
Open
Closed
@mordak mordak mentioned this pull request Apr 21, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonhoo jonhoo jonhoo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mordak @jonhoo @scottmcm