-
-
Notifications
You must be signed in to change notification settings - Fork 139
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
Send extracted links from extractor pool via channel #193
Closed
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
db14dde
Nit: Use http::statuscodes directly instead of through reqwest reexport
TimoFreiberg a4068a5
Nit: directly refer to already imported Client
TimoFreiberg 01187c9
Nit: make config passed to collect_links more consistent
TimoFreiberg efa18ab
Nit: remove unnecessary clone
TimoFreiberg b3a9e91
Send extracted links via channel
TimoFreiberg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed that the progress bar is a bit "bumpy" in the beginning. (There's a better word for what I mean, but I can't seem to find it, heh.)
Keeping the initial length of
links.len()
is not possible, of course because at this point we don't have the length (sincelinks
is a channel now, not aHashSet
), but I wonder if there is another way to avoid that "bumpy" behavior. Maybe we can get the initial length from the channel somehow to give a better estimate?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking about the initial period where no links were extracted yet? Or that, when links come streaming in, the progress bar can jump around?
I don't see a way to get the amount of current messages already waiting in the stream, did you mean using the capacity of the stream as the initial value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I meant the jumping when links start streaming in. Thought there was a
length
method formpsc
, but it doesn't exist. Probably mixed it up with another channel impl. Anyway, if we can't find a solution for the jumpiness, it's fine, too. 🤷♀️There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The downside of concurrency 😄. I think its fine when the bar represents the
n/m
checked/gathered URLs in the front, else it may be even more confusing. Only other idea would be to not show any bar until all inputs have been processed, but this example (video in OP) shows that sometimes the checker is waiting for the extractor, so no process bar at all then 😢.Should be fine as it is.