Canonicalize paths to ensure includes behave as expected when folders are involved#289
Conversation
|
I'm panicking if the canonicalization fails—an alternative would be to return a compilation error. Tell me what you'd prefer. |
| let relative = root.with_file_name(path); | ||
| if relative.exists() { | ||
| return Ok(relative.into()); | ||
| return Ok(relative.canonicalize().unwrap().into()); | ||
| } |
There was a problem hiding this comment.
Thank you so much for finding the issue, finding the source of the error, and presenting a solution all in time-span of less than an hour!
I wonder if it would better to change the order of the test (untested pseudo-code):
if let Ok(absolute) = root.with_file_name(path).canonicalize() {
if absolute.exists() {
return Ok(absolute.into());
}
}Then we don't have to care if and why canonicalizing the path didn't succeed.
There was a problem hiding this comment.
Thank you for the work on rinja!
Re: order—we definitely could, but I wonder if that may lead to confusion in case the canonicalization fails but the file exists? No error would be reported, the user would see the file being where it is expected to be yet rinja would behave as if it isn't there.
There was a problem hiding this comment.
Yes, I think you're right. It's better to tell the user that there is a problem instead of ignoring it. I like explicit error messages better than a panic, because then fallback Template implementation with an empty body is generated to suppress subsequent error messages. Could you please add a check like:
relative.canonicalize().map_err(|err| CompileError::no_file_info(format_args!("could not canonicalize path {relative:?}: {err}"), None))?I don't know when the canonicalization could fail, though, when it was successfully deemed to exists. If the resolved path is excessively long? Well, in the end it does not matter, I guess. :)
a8e8106 to
2cf65d1
Compare
Fixes #288