Fix tumblr liked-by pagination detection#586
Conversation
thomas694
left a comment
There was a problem hiding this comment.
Thanks for your PR.
Yes, something has changed on the Likes pages, which is causing the error found on the last page.
| try | ||
| { | ||
| url = nextPage.Take(Ct); | ||
| if (!nextPage.TryTake(out url)) |
There was a problem hiding this comment.
Did an error pop up here or do you just not like the Take method?
By using this overload of the TryTake method, could a side effect occur?
There was a problem hiding this comment.
Did an error pop up here or do you just not like the Take method?
The InvalidOperationException from the Take does pop out in the console when there're no pages to take anymore. It seems cleaner to check if there was something in the collection or not.
By using this overload of the TryTake method, could a side effect occur?
The difference between them is that the TryTake will immediately return if the collection is empty, while the Take method will throw an exception only if it's empty and the collection is complete. Correct me if I'm wrong, but in this case there're no other threads that populate it so if we didn't add a new page at the end of the loop we should exit it.
There was a problem hiding this comment.
... but in this case there're no other threads...
What about the tasks in the calling method? Could we use another overload of the TryTake method?
However, upon closer inspection, one might suspect that there may be another problem preventing it from executing as originally intended.
src/TumblThree/TumblThree.Applications/Crawler/TumblrLikedByCrawler.cs
Outdated
Show resolved
Hide resolved
thomas694
left a comment
There was a problem hiding this comment.
Thanks for fixing this bug.
|
I have added you to our contributors list, feel free to fix more bugs. |
Checklist
Confirm you have completed the following actions prior to submitting this PR.
Title
Give your PR a short title summarising the patch, bug fix or feature.
Description
Ensure the PR description clearly describes the problem and solution and provide as much relevant information as possible.
Issue Resolution
This Pull Request Fixes #585
Proposed Changes
Check if the regex actually matches.
New or Changed Features
Does this PR provide new or changed features or enhancements? If so, what is included in this PR?