-
-
Notifications
You must be signed in to change notification settings - Fork 183
refactor!: use ResolvedInputSource downstream of InputContent
#1840
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
not needed due to the way `InputSource` was constructed in `InputContent`. previously, this was constructed by pattern matching out of ResolvedInputSource, so the InputSource variants were guaranteed only be ResolvedInputSource variants. so, there is no loss of information and no change in functionality.
This reverts commit c631bbd.
|
Yes, I think that makes sense. |
and adds Cow to ResolvedInputSource Conflicts: lychee-lib/src/types/input/content.rs lychee-lib/src/types/request.rs lychee-lib/src/utils/request.rs
|
ResolvedInputSource now Cow as well and conflicts are fixed. I find that the 'static lifetime used on the Cow forces clones in situations that should really be a borrow. For instance, But this was already a clone before this PR, so it's probably fine for now. From a quick try, hoisting the lifetime to |
|
Great progress. Thanks @katrinafyi . And yes, we could try a shorter lifetime. I personally wouldn't mind introducing an |
it looks like this: ``` $ lychee 'non-existing/*' '*.fdsamifdsa' 'no-matches?????' empty-dir [WARN ] *.fdsamifdsa: No files found for this input source [WARN ] no-matches?????: No files found for this input source [WARN ] non-existing/*: No files found for this input source [WARN ] empty-dir: No files found for this input source 0/0 ━━━━━━━━━━━━━━━━━━━━ Finished extracting links 🔍 0 Total (in 0s) ✅ 0 OK 🚫 0 Errors ``` this is implemented as a sneaky `log::warn!` which is probably undesirable. however, the code isn't set up yet to properly pass errors from the input resolving stage up to the top level. even if it was, the "error on empty input" error should probably be put behind a flag. it would also be tricky because the reporting expects a ResolvedInputSource rather than InputSource, and these warnings arise in the process of resolving. (maybe we could slightly wind back lycheeverse#1840 to make this work) anyway, much to think about.
it looks like this: ``` $ lychee 'non-existing/*' '*.fdsamifdsa' 'no-matches?????' empty-dir [WARN ] *.fdsamifdsa: No files found for this input source [WARN ] no-matches?????: No files found for this input source [WARN ] non-existing/*: No files found for this input source [WARN ] empty-dir: No files found for this input source 0/0 ━━━━━━━━━━━━━━━━━━━━ Finished extracting links 🔍 0 Total (in 0s) ✅ 0 OK 🚫 0 Errors ``` this is implemented as a sneaky `log::warn!` which is probably undesirable. however, the code isn't set up yet to properly pass errors from the input resolving stage up to the top level. even if it was, the "error on empty input" error should probably be put behind a flag. it would also be tricky because the reporting expects a ResolvedInputSource rather than InputSource, and these warnings arise in the process of resolving. (maybe we could slightly wind back lycheeverse#1840 to make this work) anyway, much to think about.
* feat: print warning if input source matches no files it looks like this: ``` $ lychee 'non-existing/*' '*.fdsamifdsa' 'no-matches?????' empty-dir [WARN ] *.fdsamifdsa: No files found for this input source [WARN ] no-matches?????: No files found for this input source [WARN ] non-existing/*: No files found for this input source [WARN ] empty-dir: No files found for this input source 0/0 ━━━━━━━━━━━━━━━━━━━━ Finished extracting links 🔍 0 Total (in 0s) ✅ 0 OK 🚫 0 Errors ``` this is implemented as a sneaky `log::warn!` which is probably undesirable. however, the code isn't set up yet to properly pass errors from the input resolving stage up to the top level. even if it was, the "error on empty input" error should probably be put behind a flag. it would also be tricky because the reporting expects a ResolvedInputSource rather than InputSource, and these warnings arise in the process of resolving. (maybe we could slightly wind back #1840 to make this work) anyway, much to think about.
for parts of the link checking pipeline including and downstream of
InputContent, this PR replacesInputSourcefields withResolvedInputSource. specifically, this means the structsInputContent,Request, andResponse.this makes sense for two reasons:
*_contentmethods in that code snippet. so there is no loss of information in making this PR, it is just a more precise type.in particular, the second point above means this can be done with no changes to external behaviour (there is a breaking API change, of course). in fact, this PR is almost a direct text replacement, with the gain in line count only happening because of new multi-line expressions.
this PR is motivated because when working with relative URLs and base URLs, it would be nice to know that the FsGlob case is excluded, as it would be impossible to construct a URL relative to a glob.
i've tried to make this PR reasonably thorough in its replacement of usages but notably, the stats maps are still
HashMap<InputSource, ...>rather than ResolvedInputSource. i tried to change this and it led to a can of worms with InputSource implementing Deserialize but ResolvedInputSource doesn't, so i abandoned it. so, we simply convert ResolvedInputSource back to InputSource before making the statistics.one other outstanding todo might be to use Cow in ResolvedInputSource, as InputSource now does.
this PR is just some old changes I had lying around. I'm not too attached to this PR and could get by without it. it is a breaking API change, so feel free to leave it if you wish.