Skip to content
This repository was archived by the owner on Nov 8, 2022. It is now read-only.

Implement auto sending requests #8

Merged
merged 7 commits into from
Oct 1, 2020
Merged

Implement auto sending requests #8

merged 7 commits into from
Oct 1, 2020

Conversation

WaffleLapkin
Copy link
Contributor

No description provided.

@hirrolot
Copy link
Member

hirrolot commented Oct 1, 2020

I'm worried about the technical debt that this PR would introduce because it comprises of lots of tricks and quite unintuitive control flow, despite of being working.

How about using a macro instead?

[playground]

macro_rules! send {
    ($req:expr) => {
        $req.send().await
    };
}

// Our hypothetical request.
struct Request;

impl Request {
    async fn send(self) -> i32 {
        123
    }
}

fn main() {
    let _123 = async {
        let req = Request;
        send!(req)
    };
}

It's a pity that we can't do req.send!() yet...

@hirrolot
Copy link
Member

hirrolot commented Oct 1, 2020

Also related to: rust-lang/rust#67982.

@WaffleLapkin
Copy link
Contributor Author

I don't think that macro would accomplish much.

  1. Macros are a bit scary. Personally, I expect that a macro will do something complicated and not just .await.send().
  2. send!(...) vs ....send().await not much a difference (I would prefer the later...)

I also don't think that this code is too complicated. It just does simple bot-wrapper-things + implements Future for its request. Indeed this requires some minor tricks around Pin<_>, but they are ok, in my opinion.

@hirrolot
Copy link
Member

hirrolot commented Oct 1, 2020

I agree that send!(...) is not much better than ... .send().await, and moreover would necessitate deprecation of send! after the language issue above eventually resolves. In contrast, your PR plays nicely with the future directions of IntoFuture.

Concerning the PR code. I was confused that it uses unreachable expressions, but others conceived me that it's okay for Rusty futures, so nevermind.

@WaffleLapkin
Copy link
Contributor Author

Well, if IntoFuture will support async/await just as IntoIterator+for then wrapper introduced in this PR will be useless too. We'll just add : IntoFuture bound for Request.

@hirrolot
Copy link
Member

hirrolot commented Oct 1, 2020

Well, if IntoFuture will support async/await just as IntoIterator+for then wrapper introduced in this PR will be useless too. We'll just add : IntoFuture bound for Request.

I understand; I meant that it would not break backward compatibility in user's code (at least in most cases).

WaffleLapkin and others added 3 commits October 1, 2020 21:00
Co-authored-by: Temirkhan Myrzamadi <[email protected]>
Co-authored-by: Temirkhan Myrzamadi <[email protected]>
Co-authored-by: Temirkhan Myrzamadi <[email protected]>
@hirrolot hirrolot self-requested a review October 1, 2020 18:12
@hirrolot hirrolot merged commit 745d4a1 into master Oct 1, 2020
@hirrolot hirrolot deleted the requests_redisign_p3 branch October 1, 2020 18:19
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