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

Improve documentation for FromRequest::Future #2734

Merged
merged 5 commits into from Apr 23, 2022
Merged

Improve documentation for FromRequest::Future #2734

merged 5 commits into from Apr 23, 2022

Conversation

mattfbacon
Copy link
Contributor

PR Type

Other: Documentation

PR Checklist

  • [N/A] Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • [N/A] A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • [N/A] (Team) Label with affected crates and semver status.

Overview

Improve documentation for the Future associated type of the FromRequest trait. I was confused by this and made a Rust forum post where I got some help so I thought it would make sense to add this to the documentation.

No breaking changes.

@robjtede
Copy link
Member

The best practice for Actix Web is to use LocalBoxFuture instead. I'd also prefer not to mention nightly features in our docs for now.

@mattfbacon
Copy link
Contributor Author

The best practice for Actix Web is to use LocalBoxFuture instead. I'd also prefer not to mention nightly features in our docs for now.

OK

@fakeshadow
Copy link
Contributor

You can also make a new type and implement Future trait and LocalBoxFuture is just an alias for Pin<Box<dyn Future>> .The doc of actix-web should not try to offer a hand holding tutorial on basic language feature. It can get outdated easily(async Rust is still in development) and it can't do a good job on explaining it thoroughly.

@mattfbacon
Copy link
Contributor Author

mattfbacon commented Apr 18, 2022

I tried to find resources on this information and couldn't find any. And even when I asked on the forum, a Rust (specifically async; Alice is a primary maintainer of Tokio) expert said something different than what you all are saying. So it's not true that it's a "basic language feature" (it's not a language feature at all, but that's beside the point).

Anyway, I think it would be helpful. There are no examples that I could find in the docs that used it.

@mattfbacon
Copy link
Contributor Author

Sorry, part of my previous comment was incorrect. I found one example using LocalBoxFuture in one place. 😃

@fakeshadow
Copy link
Contributor

fakeshadow commented Apr 19, 2022

I'm not saying the language feature have a good discovery. I'm saying said discovery does not belong to actix-web library. Making related question more easy to get answer of from other source is better than burden some library that not maintained by rust-lang domain.
With major improvement of async Rust language feature someone have to keep the information updated and who would do that for actix-web?

@fakeshadow
Copy link
Contributor

BTW async-trait's readme have a detailed explain for this issue https://github.com/dtolnay/async-trait

@robjtede robjtede added A-web project: actix-web B-semver-norelease change that does not require a release labels Apr 19, 2022
@mattfbacon
Copy link
Contributor Author

I'm saying said discovery does not belong to actix-web library

The existence of boxed futures, no, but the Actix convention to use LocolBoxFuture yes.

With major improvement of async Rust language feature someone have to keep the information updated

If the convention changes from LocalBoxFuture to something else (maybe impl Future once that stabilizes), that should be documented.

who would do that for actix-web?

Whoever created and/or will change the convention.

@robjtede robjtede added this to the actix-web v4.1 milestone Apr 23, 2022
@robjtede robjtede enabled auto-merge (squash) April 23, 2022 20:03
@robjtede robjtede merged commit 9aab911 into actix:master Apr 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web project: actix-web B-semver-norelease change that does not require a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants