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

enable fn index(web::Path((id, name)): web::Path<(u32, String)>) #2070

Closed
wants to merge 1 commit into from

Conversation

rich-murphey
Copy link

@rich-murphey rich-murphey commented Mar 13, 2021

PR Type

Bug Fix

PR Checklist

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

Overview

Use of the Path extractor is documented in README.md, which says this is valid:
async fn index(web::Path((id, name)): web::Path<(u32, String)>) -> impl Responder {

The above fails to compile. Earlier versions of actix-web supported Path de-structuring. The example in README.md and any users' code that uses the Path de-structuring will fail to compile without this PR.

One of the motivations for using the path de-structuring, is to make it easier to review code and ensure that the mapping of path items to corresponding variables, for example:

#[get("/foo/{limit}/{offset}")]
pub async fn foo(
    web::Path((limit, offset)): web::Path<(i64, i64)>,
) -> HttpResponse {

I fee this is easier to maintain.

I'd be glad to add more tests, documentation or whatever else is needed.

@robjtede
Copy link
Member

robjtede commented Mar 13, 2021

change is intentional, see this comment
#2016 (comment)

@robjtede robjtede closed this Mar 13, 2021
@rich-murphey
Copy link
Author

The code in README.md does not compile, user's may not understand the change without some mention.

@robjtede
Copy link
Member

the code in readme is suitable for the current stable version, v3.3, and has been kept as such to reduce confusion during the beta period

@brianbruggeman
Copy link

Where can I find documentation for the beta updates?

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.

3 participants