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

Add "name" attribute to route macro #1934

Merged
merged 3 commits into from
Mar 4, 2021
Merged

Add "name" attribute to route macro #1934

merged 3 commits into from
Mar 4, 2021

Conversation

stdrc
Copy link
Contributor

@stdrc stdrc commented Jan 26, 2021

PR Type

Feature

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

This PR adds a new name attribute to the route macro, so that user can specify a resource name, which by default is the name of handler function since #1178, and call HttpRequest.url_for on the name later.

Example:

#[get("/static", name = "static")]
async fn static_file() -> impl Responder {
    // ...
}

@robjtede robjtede added C-feature Category: new functionality A-codegen project: actix-web-codegen labels Jan 26, 2021
@robjtede
Copy link
Member

Why do you feel using the handler name is not sufficient? Doesn using raw identifiers r#static not work?

@robjtede robjtede requested review from a team January 26, 2021 15:10
@stdrc
Copy link
Contributor Author

stdrc commented Jan 26, 2021

Thanks for reminding me the r#static solution, but I think this PR is still usefull if we want some special characters in the resource name. BTW, it's just a counterpart to Flask's endpoint parameter of route decorator.

@robjtede robjtede added B-semver-minor and removed C-feature Category: new functionality labels Jan 26, 2021
resource_name.value()
} else {
name.to_string()
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be a unwrap_or_else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, changed

@robjtede
Copy link
Member

robjtede commented Feb 9, 2021

would be okay to move ahead with this if you can get it rebased @richardchien

@stdrc
Copy link
Contributor Author

stdrc commented Feb 10, 2021

would be okay to move ahead with this if you can get it rebased @richardchien

Ok, I've rebased

@therealprof
Copy link
Contributor

@richardchien Needs conflict resolution now. ;)

@stdrc
Copy link
Contributor Author

stdrc commented Mar 4, 2021

@therealprof Ok, resolved

@robjtede
Copy link
Member

robjtede commented Mar 4, 2021

I can usually rebase master branch into PRs in order to coordinate the merge but you must have that option disabled. I'm sorry but I'm going to have to ask you to rebase again because another PR just got merged that fixed CI.

@stdrc
Copy link
Contributor Author

stdrc commented Mar 4, 2021

@robjtede Rebased again:)

@robjtede
Copy link
Member

robjtede commented Mar 4, 2021

@richardchien thanks, i shall merge this one next

@robjtede robjtede merged commit fc6f974 into actix:master Mar 4, 2021
@robjtede
Copy link
Member

robjtede commented Mar 4, 2021

😅 there we go... thanks for you contribution @richardchien

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen project: actix-web-codegen B-semver-minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants