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

feat: add batching in URL before fetching articles #85

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

hahaschool
Copy link
Contributor

@hahaschool hahaschool commented Jun 28, 2023

Fixes #84 . All tests passed locally.

@TomDonoghue
Copy link
Member

Thanks for the submission @hahaschool! Can you merge main in - it needs the updates to set which test versions to run

@hahaschool
Copy link
Contributor Author

Hi @TomDonoghue ,

Rebased, all tests passed locally.

Thanks.

@lisc-tools lisc-tools deleted a comment from codecov-commenter Sep 13, 2023
@TomDonoghue
Copy link
Member

Hey @hahaschool - sorry for the delay!

When returning a large number articles, you should really be using history (usehistory=True), as this stores the set of article IDs on the remote server, and then collects them in groups. This should in general avoid any issues with hitting URL limits, and is the general suggestion to collecting large numbers of articles.

That said, I think this still seems like a totally reasonable addition, and avoids any potential failures to collect a large number of articles when not using history. I did a quick check and direct refactor, but otherwise, I think we might as well add this - so I'm going to merge it in now!

@TomDonoghue TomDonoghue merged commit d8d32be into lisc-tools:main Sep 13, 2023
5 checks passed
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.

URL length limit reached when fetching many articles
2 participants