Add abillity to set path prefix #579
Conversation
| Be verbose, includes emitting access logs | ||
|
|
||
|
|
||
| -D, --dirs-first |
There was a problem hiding this comment.
Could you remove the whitespaces here? Thanks for updating the README but let's not introduce unnecessary whitespaces.
| fn serves_requests_with_path_prefix() -> Result<(), Error> { | ||
| let route = "foobar"; | ||
| let server = server(&["--path-prefix", route]); | ||
| let client = Client::new(); |
There was a problem hiding this comment.
You can probably get away without this line by using reqwest::blocking::get directly below where you need it. This is what we do in other tests and would keep this consistent.
|
|
||
| let status = client.get(server.url()).send()?.status(); | ||
|
|
||
| assert_eq!(status, StatusCode::NOT_FOUND); |
There was a problem hiding this comment.
I'd appreciate a comment here about your expectation at this point in the test.
There was a problem hiding this comment.
Sorry, i don't get it. What do you mean?
There was a problem hiding this comment.
Basically, why do you expect it to be NOT_FOUND there but OK later in the test? Give it a bit of a narrative. :)
|
I will rebase this after #508 got merged :) |
|
Wanna rebase this, I'll review and then I'll cut a release? |
| #[structopt(long = "path-prefix")] | ||
| pub path_prefix: Option<String>, |
There was a problem hiding this comment.
It is better to rename to "route" IMO to avoid a breaking change and to be distinguished from filesystem path!
| /// Use a random path prefix | ||
| pub random_path_prefix: bool, |
| let return_path = match state.path_prefix { | ||
| Some(ref path_prefix) => format!("/{}", path_prefix), |
There was a problem hiding this comment.
| let return_path = match state.path_prefix { | |
| Some(ref path_prefix) => format!("/{}", path_prefix), | |
| let return_path = match state.path_prefix { | |
| Some(ref path_prefix) => format!("/{}", path_prefix.trim_matches('/')), |
to better handle unexpected user input.
| /// Use a specific path prefix | ||
| pub path_prefix: Option<String>, |
There was a problem hiding this comment.
I prefer this to be just String, where absent route prefix is represented as empty string. This would make it easy to use when building urls.
You may want to take a look at my previous PR 7ea3beb
| #[rstest] | ||
| fn serves_requests_with_path_prefix() -> Result<(), Error> { |
There was a problem hiding this comment.
another test case should be added to tests/bind.rs:validate_printed_urls.
| let route = "foobar"; | ||
| let server = server(&["--path-prefix", route]); | ||
| let client = Client::new(); |
There was a problem hiding this comment.
can you add more user input cases? I'm thinking of something like:
#[case("foo", "/foo")]
#[case("/foo", "/foo")]
#[case("/foo/", "/foo")]
fn test(#[case] prefix_arg, #[case] accessible_url) { }
|
Closed in favor of #682 |
This PR should replace the --random-route argument with both a --path-prefix argument and a conflicting --random-path-prefix argument.
The --random-path-prefix argument should be a fully functional replacement to --random-route.
This PR reimplements the code of @mkroman from this #208 and adds a simple test
Fixes #533