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

Easily convert typed paths into URIs #790

Merged
merged 5 commits into from
Feb 28, 2022
Merged

Conversation

davidpdrsn
Copy link
Member

#[derive(TypedPath)] will now also generate TryFrom<_> for Uri for
easily converting paths into URIs for use with Redirect and friends.

Fixes #789

`#[derive(TypedPath)]` will now also generate `TryFrom<_> for Uri` for
easily converting paths into URIs for use with `Redirect` and friends.

Fixes #789
@zys864
Copy link
Contributor

zys864 commented Feb 25, 2022

The CI / test-versions (stable) (pull_request)'s command cargo test --all --all-features --all-targets failed due to the unexpected not_send.stderr which occurs on stable-1.59.

I test it on my machine. When I use stable-1.58 version, the test command successes.

But stable-1.59 may have update stderr. May be we should fix it,be the test will failed on older rust version.

Here is stable-1.59 not_send.stderr's ACTUAL OUTPUT

error: future cannot be sent between threads safely
 --> tests/debug_handler/fail/not_send.rs:4:1
  |
4 | async fn handler() {
  | ^^^^^ future returned by `handler` is not `Send`
  |
  = help: within `impl Future<Output = ()>`, the trait `Send` is not implemented for `Rc<()>`
note: future is not `Send` as this value is used across an await
 --> tests/debug_handler/fail/not_send.rs:6:13
  |
5 |     let rc = std::rc::Rc::new(());
  |         -- has type `Rc<()>` which is not `Send`
6 |     async {}.await;
  |             ^^^^^^ await occurs here, with `rc` maybe used later
7 | }
  | - `rc` is later dropped here
note: required by a bound in `check`
 --> tests/debug_handler/fail/not_send.rs:4:1
  |
4 | async fn handler() {
  | ^^^^^ required by this bound in `check`

@davidpdrsn
Copy link
Member Author

@zys864 thanks for looking into it! I did think the test failure seemed odd but forgot that a new rust came out yesterday. I'm gonna make a separate PR to fix that in a few hours (have meetings), or you're free to do it if you have the time.

@davidpdrsn
Copy link
Member Author

Hm I just thought that this actually is a breaking change. Someone could have implemented TryFrom<_> for Uri themselves which will conflict with this but I'm fine with shipping 0.2 of axum-extra and axum-macros.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Should the conversion really be fallible? With parameters being url-encoded, the only case where I could see it failing to convert is if the route itself is invalid (in which case registering it would fail too).

@davidpdrsn
Copy link
Member Author

Ah yes that's true, doesn't need to be fallible!

@davidpdrsn
Copy link
Member Author

@jplatte I've added a to_uri method to the TypedPath trait. That way its not a breaking change and we don't have to change the macro, since we can parse the Display output. What do you think?

@jplatte
Copy link
Member

jplatte commented Feb 25, 2022

I like it! Changelog / docs still reference TryFrom, otherwise lgtm.

@davidpdrsn davidpdrsn enabled auto-merge (squash) February 28, 2022 07:55
@davidpdrsn davidpdrsn merged commit da74084 into main Feb 28, 2022
@davidpdrsn davidpdrsn deleted the typed-path-try-into-uri branch February 28, 2022 08:58
davidpdrsn added a commit that referenced this pull request Mar 1, 2022
* Easily convert typed paths into URIs

`#[derive(TypedPath)]` will now also generate `TryFrom<_> for Uri` for
easily converting paths into URIs for use with `Redirect` and friends.

Fixes #789

* Use a method on the `TypedPath` trait to convert to `Uri`

* fix doc ref

* Update changelogs
davidpdrsn added a commit that referenced this pull request Mar 1, 2022
These accidentally weren't removed in #790
davidpdrsn added a commit that referenced this pull request Mar 1, 2022
These accidentally weren't removed in #790
davidpdrsn added a commit that referenced this pull request Mar 1, 2022
* axum-macros: use fully qualified Result type (#796)

* Easily convert typed paths into URIs (#790)

* Easily convert typed paths into URIs

`#[derive(TypedPath)]` will now also generate `TryFrom<_> for Uri` for
easily converting paths into URIs for use with `Redirect` and friends.

Fixes #789

* Use a method on the `TypedPath` trait to convert to `Uri`

* fix doc ref

* Update changelogs

* Remove out of date docs

These accidentally weren't removed in #790

Co-authored-by: Matthias Vogelgesang <matthias.vogelgesang@gmail.com>
davidpdrsn added a commit that referenced this pull request Mar 1, 2022
- **added:** Add `TypedPath::to_uri` for converting the path into a `Uri` ([#790])

[#790]: #790
davidpdrsn added a commit that referenced this pull request Mar 1, 2022
- **added:** Add `TypedPath::to_uri` for converting the path into a `Uri` ([#790])

[#790]: #790
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement into Uri for TypedPath
3 participants