Introduces external depth (#74) & a few fixes (incl. #69) #146
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CohenArthur
reviewed
Aug 24, 2021
Skallwar
reviewed
Aug 31, 2021
Skallwar
reviewed
Sep 6, 2021
|
You need to fix the coding style (use rustfmt) |
CohenArthur
reviewed
Sep 9, 2021
|
Please rebase on top of |
Skallwar
force-pushed
the
master
branch
2 times, most recently
from
526a5c5 to
1be1f85
Compare
Skallwar
approved these changes
Sep 10, 2021
|
@marchellodev Can you still work on this? If not, I can rebase and add tests for you |
|
@Skallwar I would really appreciate it! :) I tried to do that a few days ago, but I always stumbled upon some errors. I'm kinda new to git, especially to those sophisticated operations Thanks again! |
Introduces --edepth flag for external domain depth Transforms urls that start with `//` into full https links for more accurate detection of external links Fixes panic when url starts with `///` Readme: Added explanation of the `--edepth` flag --edepth -> --ext-depth Refactors code to avoid repetition (normalizing urls) Improved code documentation (url normalization) README.md upd: --ext-depth Minor docs changes (--depth) rustfmt coding style Updated `--ext_depth` command docs Small code documentation improvements Co-authored-by: CohenArthur <[email protected]> Co-authored-by: Esteban Blanc <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #146 +/- ##
==========================================
+ Coverage 62.54% 62.62% +0.07%
==========================================
Files 16 17 +1
Lines 558 610 +52
==========================================
+ Hits 349 382 +33
- Misses 209 228 +19
|
|
@CohenArthur are you ok with this? |
CohenArthur
approved these changes
Sep 23, 2021
|
@marchellodev Thanks again, excellent work |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
A lot of modern websites rely on external domains (usually referred to as cnd domains) for their css, js, images, and other resources. Since SuckIT does not yet support downloading data from external domains (except for the bug when
//en.wikipedia.orgis treated as a relative path, (which I fixed)), it is impossible to properly download big and complex websites (#74).Also, this patch fixes panic when trying to parse urls like
///tools.wmflabs.org/, which returnsEmpty hosterror. I encountered this trying to download wikipedia. So, I think this PR should also close #69Notes
I almost have no experience with Rust, and I haven't yet implemented tests for the changes (I'm not really sure what is the best way to do this). So, please look at the code with extra scrutiny :). However, I have tested it on a few websites, and everything seems to work properly.
Also,
--edepth(external depth) does not have a shortcut, since-eis used for excluding pattern. I'm not sure how this parameter should be renamed in order for shortcut to exist