Skip to content

send unsolicited responses to a channel instead of discarding them #91

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

Merged
merged 3 commits into from
Nov 1, 2018

Conversation

dario23
Copy link

@dario23 dario23 commented Oct 29, 2018

re #72 discussion

notes

  • i tried to avoid the term "async", because that term is very
    overloaded and we're not using e.g. tokio/async-io here
  • i'm a little unhappy having to string the channel through the
    parser, because that seems rather a part of the client logic than
    parsing. on the other hand it's better than passing the whole
    client, so there's that at least.

@jonhoo
Copy link
Collaborator

jonhoo commented Oct 29, 2018

This is great! Any particular reason you are cloning the Sender everywhere as opposed to passing &mut self.unsolicited_responses_tx?

dario23 added a commit to dario23/rust-imap that referenced this pull request Oct 29, 2018
@dario23
Copy link
Author

dario23 commented Oct 29, 2018

This is great! Any particular reason you are cloning the Sender everywhere as opposed to passing &mut self.unsolicited_responses_tx?

good point, changed that part, and fixed the syntax error from the previous commit :-)

@jonhoo
Copy link
Collaborator

jonhoo commented Oct 30, 2018

I'd also love to see a test that does test for an unsolicited response, as opposed to only ones that don't, if you can spare the time!

@jonhoo
Copy link
Collaborator

jonhoo commented Nov 1, 2018

Awesome, thanks! I also recently moved to nom 4 in 5c91f4c, so you'll have to rebase onto master (or a merge is also fine if you're not comfortable with git rebases) to make things work again. The change is mostly mechanical:

IResult::Done(rest, data)

just becomes

Ok((rest, data))

notes

  * i tried to avoid the term "async", because that term is very
    overloaded and we're not using e.g. tokio/async-io here
  * i'm a little unhappy having to string the channel through the
    parser, because that seems rather a part of the client logic than
    parsing. on the other hand it's better than passing the whole
    client, so there's that at least.
@dario23
Copy link
Author

dario23 commented Nov 1, 2018

  • added some tests, also some more handling along the way because tests found me being a little sloppy :-)
  • rebased onto latest master (i like merges better when a branch was developed separately for some time and is later adopted into master, but for ongoing development like this i find linear history more aesthetic)

@jonhoo jonhoo merged commit f02950a into mattnenterprise:master Nov 1, 2018
@jonhoo
Copy link
Collaborator

jonhoo commented Nov 1, 2018

Fantastic, thank you!

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