Fix the wget footer #1043
Fix the wget footer #1043
Conversation
|
Please squash your commits. |
|
Fixes #1039. |
Yusuto
force-pushed
the
wget-fix
branch
2 times, most recently
from
4389d7b
to
a836b46
Compare
I think I squashed my commits, I hope I did what you mean and didn't mess anything up with the force pushes Edit: Read some more in the Git Book and I was literally just one chapter away from where it explained how to squash commits properly, so I could have done that instead of the weird git-voodoo that article I found made me do >< |
...by having it use the absolute url instead of the breadcrumbs
|
It's not necessary for this MR btw, but if you'd like to, you could write a small test to make sure the behavior for this footer is always functional. :) |
Oh I'd love to do that Yip! Also I couldn't add the tests right now, should I add them in a separate PR, add there here later (squashed or unsquashed?), or for now not at all? |
|
You could also add the tests in this MR. Don't worry, I'll wait. I just want to release something this week ideally. I think in this case a unit test of this particular function in the same file makes sense as well as an integration test. we have plenty of examples for both so it shouldn't be that hard. :) |
|
Yip, sounds gud, Thanks! I'll get to it either later today or tomorrow morning ☀️ |
The command now has slightly more concise flags, the cut_dir flag gets omitted when it would be zero, if downloading root put files in a folder named the title of the page, use match claues instead of ifs, bit more concise I think
|
Oh also I just notice this breaks a bunch of tests, I'll adjust them to check for the new behavior |
I honestly just switched the definition and then frantically changed small things based on rusts error messages, but it passes fmt, clippy and tests so I think it's fine. This allow a bit finer control over the URI, but is honetly a bit insignificant.
|
I'm done I think!
If any issues arise I'll get to them tomorrow, but if nawt then I think this MR is ready to merge! |
Yusuto
force-pushed
the
wget-fix
branch
2 times, most recently
from
44717ab
to
4547dc9
Compare
src/renderer.rs
Outdated
| fn test_wget_footer() { | ||
| let to_html = |x| { | ||
| format!( | ||
| r#"<div class="downloadDirectory"><p>Download folder:</p><a class="cmd" title="Click to copy!" style="cursor: pointer;" onclick="navigator.clipboard.writeText("wget -rcnHp -R 'index.html*' {0}/?raw=true'")">wget -rcnHp -R 'index.html*' {0}/?raw=true'</a></div>"#, | ||
| x | ||
| ) | ||
| }; | ||
| let uri = |x| Uri::try_from(x).unwrap(); | ||
|
|
||
| let to_be_tested: String = wget_footer( | ||
| &uri("https://github.com/svenstaro/miniserve/"), | ||
| Some("Miniserve"), | ||
| None, |
There was a problem hiding this comment.
Why not make all of these test cases into their own unit tests? That way, you can also make it self-documenting by varying the test names.
|
Hey, do you plan on fixing up the rest soonish? I'd like to do a release. No worries if you need longer but then it might not make the release. |
...discovered when writing the tests. Ran rustfmt, clippy::all, cargo test, everything passed and I hope the tests I wrote are good. Now with 100% less forgotten debug logs!
Oh, I'm not exactly sure what is left to be fixed, it's only the tests, right? I wrote a comment earlier that I'm very unsure about how to split them apart, whatever approach I try seems to be just kind of not really optimal, or at least not significantly better than just lumping it all in one function and if something fails manually figuring out what it is, so I'm a bit at loss of how to continue there I'm afraid |
|
Merging as is, thanks! I split the tests the way I imagined it. |
|
Oké, Thank you! This is my first MR I ever made, I'm very happy to get it accepted ^^ |
|
Thanks for the contribution :) I'd be happy for more contributions of course if you find something you'd like to change. |
|
No problem! |
...by having it use the absolute url instead of the breadcrumbs.
I think the changes should be rather self-explanatory, I tested my changes before opening this PR and also ran Clippy and rustfmt and neither said anything, but please still check what I did, just in case.