feat: support relative --root-dir#1912
Conversation
done by canonicalizing the root-dir at the point where the is_absolute check was previously done. this is the quick-n-fast way of making this change, as it maintains all following program invariants - the root-dir is always absolute past that point. the more sophisticated approach would allow relative root-dirs to pass unimpeded and it should all "just work" down the line. but i didnt want to take the risk a side-effect of this change is that a non-existing root-dir is now a CLI error. previously, this would cause link checking errors when it came time to check relative links which used the non-existing root-dir.
|
Ugh, ok, there is a edge case here and it is the behaviour of --root-dir + --base-url (my enemy). The path canonicalization of root-dir contradicts its role when used alongside base-url. When a base-url is present, root-dir should be taken to be relative to base-url rather than relative to the current working directory. Edit: This gives me a thought. root-dir is currently required to be an absolute filesystem path, but it can be joined onto base-url which is a URL. What happens when you are on Windows and you do |
this creates a hole where a non-absolute root-dir may slip through if a base-url is specified. this is PROBABLY fine, since it gets joined onto base url anyway
I'm so sorry. 😅 |
|
The "easiest" way would be to throw an error if we detect this on Windows. The downside, of course, is that lychee would behave differently on Windows as it does on other platforms. We could use In general, isn't it always a problem (on all platforms) if |
|
Honestly? I was actually avoiding thinking about it aha. My view is that this PR does not make the situation worse and I planned to refactor/rewrite all of this code anyway, when I eventually tackle the "fix relative links" task. I do agree that there is a conceptual problem here on all platforms, so I think it would be a bit unfair to single out Windows with a special error case. Also, after the base-url change, I'm more reluctant to add errors now :") There is a problem, but I don't know what the best approach would be. |
|
True. Do you mind if we merge this in its current form? It's definitely a much-anticipated feature. We could mention the edge-case in the release notes and maybe in our docs? |
|
No I don't mind. Maybe the edge case could gain a warning message as well. Just checking, by edge case you mean absolute path on Windows + base url? |
|
Yes, good idea, let's add a warning and then we can merge this. Can you add it? 👍 |
|
Actually, I am struggling to find words for this warning. I think it's hard to say anything concrete, and a warning like "Using absolute --root-dir alongside --base-url can be surprising" is not terribly helpful. I also don't have a Windows to test on. Maybe we just merge it w/o the warning for now. I think that if they misuse it, it will also be obvious - local links will start failing very frequently. |
|
Sure! |
|
Merged it in. I also don't know how to phrase it right now, but my key insight of this PR is that absolute paths don't make semantic sense when you're also specifying a base URL, since the root-dir needs to be relative to that URL. Based on that, I was thinking of a few options for an error message. Maybe a simple one? Error: --root-dir must be a relative path when used with --base-url
Found absolute path: {root_dir}
Hint: Use a relative path like 'docs' instead of an absolute pathOr one which better explains the conflict? Error: Cannot use absolute --root-dir with --base-url
The --root-dir path '{root_dir}' is absolute, but when --base-url is set,
--root-dir should be relative to the base URL, not the filesystem.Or a bit more precise? Error: Absolute --root-dir is incompatible with --base-url
Found: {root_dir}
When using --base-url, --root-dir must be a relative path that will be
joined to the base URL. Absolute filesystem paths cannot be combined with URLs.Thoughts? |
|
I like those, the second one in particular, but I'm also trying to think big picture and idk if the warning makes sense with that in mind. The problem happens because root-dir gets joined onto base-url. I think that this joining is not the right semantics, and we should just change that semantics rather than artificially steering users away with warnings. My long term goal is to have root-dir and base-url behave like this (from #1718 (comment)):
This disentangles them from each other and stops joining one onto the other. If the flags worked this way, then there would be no issue with an absolute root-dir and a base-url at the same time. Basically, I'm hesitant to add a warning which is only a problem because of this weird base-url + root-dir interaction. A warning which teaches the user about the current semantics could get in the way of changing those semantics. But idk, maybe it's wrong to think this far in advance. I've stalled out on implementing this base-url + root-dir change - the core is there but the tests are not. I'm also implicitly assuming that changing the existing flags' semantics is what will happen. Alternatively, we could add new options for the new behaviour. This is all into the future. Idk aha |
@katrinafyi with PHP (hopefully having experience with it is not disqualifying for having an opinion 😄 ) they just treat Windows paths as if they're also regular, so
Yes, exactly. You pass it a folder (which is automatically treated as Edit: forgot we already talked about this, sorry for the noise. |
|
Hi @dkarlovi! The issue with which is very weird and almost surely wrong. (But I can't verify because I don't own windows). On Unixes, this just happens to work because the filesystem path looks close enough to a URL. Windows is not so lucky :-) And yes, we have talked about this before! I've made some progress and it works well, but it just needs a chunk of cleanup before I push it for review. Unfortunately, I haven't had the time 😔 and I have gotten distracted with easier tasks (like this PR!). Edit: also, idk php but I do respect it :) certainly not disqualifying for opinions |
Yes, yes, this is what happens. I have seen that on Windows before. |
|
Sorry to potentially necroing this discussion: I don't understand why What my understanding of That way, If an absolute path as root dir is just attached to the base URL, then I agree |
|
Yeah. We seem to be on the same page now. The things you do not understand are the same things I do not understand. Your thinking for what it should be is slightly different to mine, however. I think that it should be the converse, so base URL is trimmed from applicable URLs, then that is appended to root dir. In this way, when the user specifies root-dir + base-url, it is to say that every subpath of base-url should be available within root-dir. This behaviour would roughly equal This behaviour is useful because it sometimes happens that you want a fully qualified link which to resolve to your local files (e.g., #1918). It still keeps root-dir and base-url separate and never joins them together. |
|
Let's continue discussion in #1718, instead of a closed PR. |
done by canonicalizing the root-dir at the point where the is_absolute check was previously done. this is the quick-n-fast way of making this change, as it maintains all following program invariants - the root-dir is always absolute past that point.
the more sophisticated approach would allow relative root-dirs to pass unimpeded and it should all "just work" down the line. but i didnt want to take the risk
a side-effect of this change is that a non-existing root-dir is now a CLI error. previously, this would cause link checking errors when it came time to check relative links which used the non-existing root-dir.
with luck, closes #1606.