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.

Sign up for GitHub

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

Jump to bottom

Implement path extractor. #124

Merged
merged 1 commit into from
Aug 6, 2021
Merged

Implement path extractor. #124

merged 1 commit into from
Aug 6, 2021

Conversation

sunli829
Copy link
Contributor

@sunli829 sunli829 commented Aug 5, 2021 •
edited by davidpdrsn

Fixes #42

@sunli829 sunli829 marked this pull request as draft August 5, 2021 03:35
sunli829 added a commit to sunli829/axum that referenced this pull request Aug 5, 2021
@sunli829
Implement path extractor and remove extract::UrlParams and `extract…
b358041 
…::UrlParamsMap`. (tokio-rs#124)
@sunli829 sunli829 changed the title Implement path extractor and remove extract::UrlParams and extract::UrlParamsMap. Implement path extractor. Aug 5, 2021
@sunli829
Copy link
Contributor Author

sunli829 commented Aug 5, 2021 •
edited

I want to replace UrlParams and UrlParamsMap with Path in all examples. What do you think? @davidpdrsn

And I did not mark UrlParams as deprecated, because this will also cause a lot of warnings, so I am going to leave this to you. 🙂

@davidpdrsn
Copy link
Member

I want to replace UrlParams and UrlParamsMap with Path in all examples. What do you think?

Yeah sounds great!

And I did not mark UrlParams as deprecated, because this will also cause a lot of warnings, so I am going to leave this to you.

👍

@davidpdrsn davidpdrsn added the C-enhancement Category: A PR with an enhancement label Aug 5, 2021
@sunli829
Copy link
Contributor Author

sunli829 commented Aug 5, 2021

I also modified all the tests for UrlParams and UrlParamsMap because they are no longer needed.

sunli829 added a commit to sunli829/axum that referenced this pull request Aug 5, 2021
@sunli829
Implement path extractor. (tokio-rs#124)
696eee5
@sunli829
Copy link
Contributor Author

sunli829 commented Aug 5, 2021 •
edited 

/// Note that you can only have one URL params extractor per handler. If you
/// have multiple it'll response with `500 Internal Server Error`.

This is a description copied from UrlParams, but I found that there is an extracting_url_params_multiple_times test, so is it allowed to extract multiple times or only once?

sunli829 added a commit to sunli829/axum that referenced this pull request Aug 5, 2021
@sunli829
Implement path extractor. (tokio-rs#124)
eed457b
sunli829 added a commit to sunli829/axum that referenced this pull request Aug 5, 2021
@sunli829
Implement path extractor. (tokio-rs#124)
51c57c9
@sunli829
Copy link
Contributor Author

sunli829 commented Aug 5, 2021

I think I have done it, and did not directly copy the de.rs of actix-web. In fact, I understood the code and built it by hand, so I don't think it is necessary to add their license.

@sunli829 sunli829 marked this pull request as ready for review August 5, 2021 09:40
sunli829 added a commit to sunli829/axum that referenced this pull request Aug 5, 2021
@sunli829
Implement path extractor. (tokio-rs#124)
392bae7
sunli829 added a commit to sunli829/axum that referenced this pull request Aug 5, 2021
@sunli829
Implement path extractor. (tokio-rs#124)
c0dd4d2
@davidpdrsn
Copy link
Member

Great work! I'll review this later today.

sunli829 added a commit to sunli829/axum that referenced this pull request Aug 6, 2021
@sunli829
Implement path extractor. (tokio-rs#124)
f6e5cd9
davidpdrsn
davidpdrsn reviewed Aug 6, 2021
View reviewed changes
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

I'm really excited for this! Using Path is so much nicer than UrlParams and UrlParamsMap!

I had a few minor comments but otherwise I think this looks good. Great job!

src/extract/path/mod.rs Outdated Show resolved Hide resolved
src/extract/path/mod.rs Outdated Show resolved Hide resolved
src/extract/path/de.rs Outdated Show resolved Hide resolved
src/extract/path/de.rs Outdated Show resolved Hide resolved
src/extract/path/de.rs Outdated Show resolved Hide resolved
@sunli829
Copy link
Contributor Author

sunli829 commented Aug 6, 2021

I have changed these. 🙂

@davidpdrsn
Copy link
Member

By the way small thing: I would prefer if you didn't force push. It makes it a little easier to review exactly whats changed in the commits 😊

@davidpdrsn davidpdrsn merged commit 9fdbd42 into tokio-rs:main Aug 6, 2021
@sunli829
Copy link
Contributor Author

sunli829 commented Aug 6, 2021

Well, I will not force push again after your review next time. 🙂

davidpdrsn added a commit that referenced this pull request Aug 6, 2021
@davidpdrsn
Version 0.1.3
358ec2d 
0.1.3 (06. August, 2021)

- Fix stripping prefix when nesting services at `/` ([#91](#91))
- Add support for WebSocket protocol negotiation ([#83](#83))
- Use `pin-project-lite` instead of `pin-project` ([#95](#95))
- Re-export `http` crate and `hyper::Server` ([#110](#110))
- Fix `Query` and `Form` extractors giving bad request error when query string is empty. ([#117](#117))
- Add `Path` extractor. ([#124](#124))
- Fixed the implementation of `IntoResponse` of `(HeaderMap, T)` and `(StatusCode, HeaderMap, T)` would ignore headers from `T` ([#137](#137))
- Deprecate `extract::UrlParams` and `extract::UrlParamsMap`. Use `extract::Path` instead ([#138](#138))
@davidpdrsn davidpdrsn mentioned this pull request Aug 6, 2021
Merged
davidpdrsn added a commit that referenced this pull request Aug 6, 2021
@davidpdrsn
Version 0.1.3 (#139)
e13f1da 
- Fix stripping prefix when nesting services at `/` ([#91](#91))
- Add support for WebSocket protocol negotiation ([#83](#83))
- Use `pin-project-lite` instead of `pin-project` ([#95](#95))
- Re-export `http` crate and `hyper::Server` ([#110](#110))
- Fix `Query` and `Form` extractors giving bad request error when query string is empty. ([#117](#117))
- Add `Path` extractor. ([#124](#124))
- Fixed the implementation of `IntoResponse` of `(HeaderMap, T)` and `(StatusCode, HeaderMap, T)` would ignore headers from `T` ([#137](#137))
- Deprecate `extract::UrlParams` and `extract::UrlParamsMap`. Use `extract::Path` instead ([#138](#138))
@johannescpk
Copy link
Contributor

johannescpk commented Aug 6, 2021 •
edited

It makes it a little easier to review exactly whats changed in the commits

Since GitHub doesn't make it obvious and some people seem to not know about: It's possible to click on the "force-push" text to see the diff of the force-pushed changes. Tho that doesn't replace follow-up changes with a commit message of course

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidpdrsn davidpdrsn davidpdrsn approved these changes

Assignees
No one assigned
Labels
C-enhancement Category: A PR with an enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extractor similar to actic_web::web::Path
3 participants
@sunli829 @davidpdrsn @johannescpk