Resolve Relative links

[RyanGreenup]

This improves the link resolution to also resolve relative links.

This is done by:

1. Check if a link is relative (using .is_relative())
   2. Join the two file path and the relative link, e.g. PathBuf::from("/home/user/Notes/Code/index.md").join("../Linux/index.md")
   3. Looping over the components of the path to drop ..
      • The .canonicalize() method is not appropriate here as it tries to resolve symlinks and panics when the file doesn't exist.

Consideration

We could simply call the .canonicalize() method and leave the link alone if that fails:

if file_path.is_relative() {
    clean_path = path.join(file_path).canonicalize().unwrap_or_else(|_| file_path).to_str();
}

• Justification
  • If there's no file, there's no need to have a working link.
• Pros
  • Simpler more readable code
  • Code won't break if the std::path::Comonent enum changes
• Cons
  • User's may expect functioning links even when there aren't files to render
    • I can't imagine such a use case but I'm sure a user will find it.

[RyanGreenup]

I pushed without running the tests and they failed.

The is_dir() requires the directory to exist on the fs and otherwise returns false, I adjusted the tests to reflect that behaviour and added it to the docstring of the function.

[shaira-greenup]

The .is_relative() will always be true. Maybe test for component::Parent in file_path?

[RyanGreenup]

Irrelevant

> The `.is_relative()` will always be `true`. Maybe test for `component::Parent` in `file_path`?i Ahh, that's a good point, I just fixed that `53d2b84..816de55`, now it only applies this logic if the path contains a reference to a parent directory. The only problem is that the relative links become absolute paths because I think `dest_url` takes the current file (`path`) as an absolute path and target links (`file_path`) as relative links. We can truncate `file_path` back down to a relative path: ```rust fn try_truncate_cwd(file_path: &PathBuf) -> Option { let current_dir = std::env::current_dir().ok()?; let file_path = PathBuf::from(file_path); Some( file_path .strip_prefix(¤t_dir) .ok()? .into_iter() .map(|p| p.to_owned()) .collect(), ) } fn test_try_truncate_cwd() { let file_path = std::env::current_dir().unwrap().join("file.md"); assert_eq!(try_truncate_cwd(&file_path), Some(PathBuf::from("file.md"))); let file_path = std::env::current_dir().unwrap().join("foo/bar/baz/file.md"); assert_eq!( try_truncate_cwd(&file_path), Some(PathBuf::from("foo/bar/baz/file.md")) ); } ``` However we can't know when to do that as `path.is_absolute()` is always true in that part of the code (unless there's another variable to look at?). We could make all paths relative to the current working directory but this would break links that are not under the current working directory, this would be a breaking change from current behaviour. I think all links should be relative to the current working directory, this is typical of other servers and It's hard to invision a use case where a user couldn't simply run the server from that directory anyway.

[RyanGreenup]

The .is_relative() will always be true. Maybe test for component::Parent in file_path?

At first I was using that to check for ../ as you pointed out, but I re-considered and now the logic is:

• Is the link relative?
  • false: Resolve based on absolute path, this would be useful for e.g. vim opening notes from in a different working directory
  • true:
    • Is the link under the working directory: false: Resolve based on absolute path true: Resolve based on the relative path, this way the user doesn't see long URL

I'm happy with this now if you wanted to pull. Alternatively, if there's any feedback, I'm happy to fix.

[elijah-potter]

This PR is pristine. Thank you for all the work you put into this. Approved.