Append fallback extensions when the pre-existing extension has a space#2236
Append fallback extensions when the pre-existing extension has a space#2236katrinafyi wants to merge 8 commits into
Conversation
mre
left a comment
mre
left a comment
There was a problem hiding this comment.
Thanks. I think the fix looks fine and it would unblock a few users, so I'm happy to merge it.
To your point about appending extensions, I agree that it might break a bunch of use-cases and in general, it would be less intuitive than the current behavior. E.g. as a user, I'd expect the extensions to be exactly the ones I specified in the option, because otherwise I'd have to look at the docs or the source code to find out what's going on. 😉
| && ext.as_encoded_bytes().iter().any(u8::is_ascii_whitespace) | ||
| { | ||
| let mut ext = ext.to_os_string(); | ||
| ext.push(".extra-extension"); |
There was a problem hiding this comment.
That's perhaps the only little wrinkle. I'm guessing you didn't want to refactor the code too much to fix that issue? It's okay to merge, but can we add a comment about that so we know that we should refactor that in the future?
There was a problem hiding this comment.
I've changed it to avoid the magic string in https://github.com/lycheeverse/lychee/pull/2236/changes/bf010683fb998d148938f7fca89e37a27505723d..caa4e985d434facd15c2c7579754bf02a05c2c0b, let me know if that's better.
And yeah, I wanted to reuse the existing code as much as possible so I just append a new extension first to avoid overwriting the space-containing extension.
2 participants
Fixes #2235 but I'm not totally sure whether this fix is right.
Maybe fallback-extensions should always just append the fallback extension rather than trying to replace a pre-existing extension? That would be a neater fix to this bug. But maybe some use cases would want a link like
x.htmlto fall back to ax.mdfile (for example, when doing markdown to html generation). Not sure. In any case, I've added some test cases which show the behaviour.