Introduce --root-dir #1576
Introduce --root-dir #1576
Conversation
|
Also, I don't understand the (remaining) CI failures if you could give some hint, thanks! Update: there are only lint failures remaining now which seem unrelated to this PR, though I'm probably missing something |
|
Hey @trask, this is a very solid attempt and a very long-standing issue of the project, so thanks a lot for looking into that! I haven't checked all the edge-cases yet, but I think you're on the right track. On another note, we could use the opportunity and deprecate But we don't have to do that in one go. Let's take it step by step. |
|
Thanks for the feedback! I've incorporated your suggestions. |
|
Awesome. Thanks! From my side, this is good to go. Let's wait for the feedback of at least one other person and then we can merge it. |
|
@george-gca, any updates? Did you find the time to give it a try? |
|
Code-wise it looks good. But now I'm confused about its usage. When I'm in the fixtures directory I do: But: @trask Why isn't the first one working? Why is the second working? |
relative
and so when e.g. running
turns the absolute local link and running
turns the absolute local link It probably makes more sense to resolve relative |
|
Good point. An alternative would be to prevent relative links by throwing an error. It would be simpler to implement while avoiding ambiguity and unwanted side effects. The help text would need to be updated to indicate that an absolute directory is expected. We could start with a conservative design and allow for future support of relative paths if we see a use-case. |
done! |
|
@thomas-zahner, what do you think? Can you try it again? |
|
Wouldn't it be better to accept both absolute and relative path, and handle them internally? Maybe doing something like suggested in these answers on stackoverflow? One uses PathBuf itself with std::fs::canonicalize, the other would add a new dependency path-clean. |
|
Ah, sorry. It's not applicable for your case because you have a more complex routing setup. |
lychee-bin/src/main.rs
Outdated
| let mut collector = Collector::new(opts.config.base.clone()) | ||
| if let Some(root_dir) = &opts.config.root_dir { | ||
| if root_dir.is_relative() { | ||
| bail!("`--root_dir` must be an absolute path"); |
There was a problem hiding this comment.
One last thing; I think we should move this to Collector::new. lychee can also be used as library through lychee-lib so that's another entrypoint where this invariant should be enforced
| impl Collector { | ||
| /// Create a new collector with an empty cache | ||
| #[must_use] |
There was a problem hiding this comment.
clippy told me to remove this:
warning: this function has a
#[must_use]attribute with no message, but returns a type already marked as#[must_use]
Unfortunately I haven't been at the company at which we were using this for more than one year now :') They've also moved the specs this was to be used on, so it's hard to say. |
|
Great! Thanks for your work @trask! 👏 |
|
I changed my mind regarding relative path handling for # ✅ Good
lychee --root-dir "$(pwd)/public"
# ❌ Bad: will fail
lychee --root-dir ./public/Maybe we should implement what @george-gca suggested all along: we simply always canonicalize the use std::fs::canonicalize;
use std::path::PathBuf;
fn main() {
let path = PathBuf::from(".");
println!("{:?}", canonicalize(path));
}@trask, @george-gca, @thomas-zahner, I've created a separate issue for that here: #1606. |
Related to #452 (comment)
I tried a couple of more ambitious refactoring, but they kept spiraling out of (my) control, so tried to keep it minimal.