Skip to content
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

Add skip_missing flag, add Input enum (closes #26) #35

Merged
merged 12 commits into from
Dec 2, 2020

Conversation

pawroman
Copy link
Member

@pawroman pawroman commented Nov 24, 2020

Handle input arguments in a more structured way, with the ability to skip missing files.

Done:

  • Implementation of Better handle input files arguments #26
  • Ensure skip_missing flag is handled properly (the default behavior has been changed to error if one of the input files is missing (doesn't apply for globs or URLs))
  • Improve docs for input types
  • Refactor inputs to be more generic and extensible
  • Parallelize the collect_links method for faster operation with multiple inputs/files
    • The next step (some time in the future) could be to dispatch each link for checking as soon as it's extracted, making the program much faster for large amounts of inputs and links
  • Tests for all input types handling, including integration tests

@pawroman pawroman force-pushed the better-handle-files branch from 0b8b089 to 3ac68b8 Compare November 25, 2020 18:44
@mre
Copy link
Member

mre commented Dec 1, 2020

Draft looks good to me. 👍

@mre mre mentioned this pull request Dec 1, 2020
@pawroman pawroman marked this pull request as ready for review December 1, 2020 19:03
src/main.rs Outdated Show resolved Hide resolved
@pawroman
Copy link
Member Author

pawroman commented Dec 1, 2020

@mre It took way longer than I thought, but it should be OK to go now

README.md Show resolved Hide resolved
src/collector.rs Show resolved Hide resolved
src/collector.rs Show resolved Hide resolved
src/collector.rs Show resolved Hide resolved
src/collector.rs Show resolved Hide resolved
src/collector.rs Show resolved Hide resolved
src/collector.rs Outdated Show resolved Hide resolved
src/collector.rs Show resolved Hide resolved
src/collector.rs Show resolved Hide resolved
}
}

impl<P: AsRef<Path>> From<P> for FileType {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice use of AsRef and Default above.

src/extract.rs Outdated Show resolved Hide resolved
src/options.rs Show resolved Hide resolved
tests/cli.rs Show resolved Hide resolved
tests/cli.rs Show resolved Hide resolved
@mre
Copy link
Member

mre commented Dec 1, 2020

Very good PR! Thanks for tackling this @pawroman. I really like how you improved code quality across the board and paid attention to the smallest details like fixing comments and making the code more idiomatic. Thanks for that.
I just added a few veeery minor notes but we don't have to tackle any of them right now. Just would like to get your feedback on this at some point, but feel free to merge right away if you like. 😃

@pawroman pawroman force-pushed the better-handle-files branch from 34b68af to 3b0d811 Compare December 2, 2020 18:05
Also add max_concurrency to collect_links
src/extract.rs Show resolved Hide resolved
src/options.rs Show resolved Hide resolved
@mre
Copy link
Member

mre commented Dec 2, 2020

So happy to merge this. A major step forward and built with great attention to detail. Looking forward to whatever you'll tackle next.

@mre mre merged commit 1f78761 into lycheeverse:master Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants