Improve link fallback #582
Conversation
| @@ -111,7 +111,7 @@ impl CratesfyiHandler { | |||
|
|
|||
| let shared_resources = Self::chain(&pool_factory, rustdoc::SharedResourceHandler); | |||
| let router_chain = Self::chain(&pool_factory, routes.iron_router()); | |||
| let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").unwrap()).join("public_html"); | |||
| let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").expect("CRATESFYI_PREFIX environment variable does not exists")).join("public_html"); | |||
There was a problem hiding this comment.
What do you think, is this overkill?
| let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").expect("CRATESFYI_PREFIX environment variable does not exists")).join("public_html"); | |
| let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX") | |
| .expect("the CRATESFYI_PREFIX environment variable is not set, \ | |
| see https://github.com/rust-lang/docs.rs/wiki/Developing-without-docker-compose \ | |
| for instructions on developing locally") | |
| ).join("public_html"); |
There was a problem hiding this comment.
The issue I feel with putting the link on the expect is that it is weird for this one to have it, and not the other ones, but if all of them have it, it gets pretty ugly, so maybe it needs to just check all the environment variables at the start, and print an error message, but I don't think it should be put into this pull request.
There was a problem hiding this comment.
| let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").expect("CRATESFYI_PREFIX environment variable does not exists")).join("public_html"); | |
| let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").expect("the CRATESFYI_PREFIX environment variable is not set")).join("public_html"); |
src/web/rustdoc.rs
Outdated
| req_path.push("index.html"); | ||
| } else if !end.contains('.') { |
There was a problem hiding this comment.
This is not the solution I would like to see. If there is a file (not a directory) called /:crate/:version/name, this will prevent it from being seen, instead always redirecting to the directory. I would rather fallback to the directory in
Line 254 in 777b37c
Also, this is a bit of a fragile part of the code and I'd rather not change it unless absolutely necessary.
| //If it is a directory, we default to looking | ||
| if path.ends_with("/") { | ||
| path.push_str(DIR_DEFAULT_FILE); | ||
| req_path.push(DIR_DEFAULT_FILE); |
There was a problem hiding this comment.
I don't think this is the same behavior, it will turn /crate/version/ into /crate/version//index.html.
This will mess up the redirect to latest version, which unfortunately only has unit tests in #570.
Since that's the case, I'm not comfortable merging until either #570 is merged or I get a chance to test locally which will probably be a few days.
If you instead change only the from_path block (line 251), I would be more comfortable with the changes.
| @@ -315,7 +318,7 @@ fn path_for_version(req_path: &[&str], target_name: &str, conn: &Connection) -> | |||
| return req_path[3..].join("/"); | |||
| } | |||
| // this page doesn't exist in the latest version | |||
| let search_item = if *req_path.last().unwrap() == "index.html" { | |||
| let search_item = if *req_path.last().unwrap() == DIR_DEFAULT_FILE { | |||
There was a problem hiding this comment.
Is there a reason you made this change?
| .rustdoc_file("all.html", b"some data 2") | ||
| .rustdoc_file("file_without_ext", b"some data 3") | ||
| .rustdoc_file("some_module/directory/index.html", b"some data 3") | ||
| .rustdoc_file("some_module/file_without_ext", b"some data 3") |
There was a problem hiding this comment.
Please also test /directory.html/index.html, which I don't think will work currently.
| @@ -111,7 +111,7 @@ impl CratesfyiHandler { | |||
|
|
|||
| let shared_resources = Self::chain(&pool_factory, rustdoc::SharedResourceHandler); | |||
| let router_chain = Self::chain(&pool_factory, routes.iron_router()); | |||
| let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").unwrap()).join("public_html"); | |||
| let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").expect("CRATESFYI_PREFIX environment variable does not exists")).join("public_html"); | |||
There was a problem hiding this comment.
| let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").expect("CRATESFYI_PREFIX environment variable does not exists")).join("public_html"); | |
| let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").expect("the CRATESFYI_PREFIX environment variable is not set")).join("public_html"); |
Improve handling of urls that are missing a /
Ex: http://127.0.0.1:3000/hyper/0.13.2/hyper/client/conn
Fix #288