-
Notifications
You must be signed in to change notification settings - Fork 115
Fix GitHub.commented_in_range
#444
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
Conversation
c06a3b6 to
d28a3fc
Compare
|
Dear @sandrobonazzola, @kwk, and @psss, did you have time to review this improvement? |
sandrobonazzola
left a comment
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.
Some inline comments
|
Thanks for the review, it's ready for the next round. |
01e1907 to
d666b8a
Compare
|
Here's an example of executed DetailsIt works longer, but finds more commented issues and PRs |
|
I added another condition to the search query It's just a bit longer than the master's code, but it finds all the issues and PRs from the last year. And another comparison, the latest commit VS master, 20-30 entities difference: Details |
|
Hello! Can I kindly request another review round? |
- Add `?per_page={PER_PAGE}&since={since}` to not fetch old comments
- If the latest comment on a page is older than `until`, fetch another page
6e12bab to
7da227c
Compare
Unfortunately, the current code doesn't cover many edge cases for large/old PRs and issues. Here are the cases that I address in the PR:
until, it will be filtered out by the firstsearchquerysinceparameter isn't used. For huge issues/PRs, it helps to reduce the number of requests significantly