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

ArxivAPIWrapper - doc_content_chars_max #6063

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

hp0404
Copy link
Contributor

@hp0404 hp0404 commented Jun 12, 2023

This PR refactors the ArxivAPIWrapper class making doc_content_chars_max parameter optional. Additionally, tests have been added to ensure the functionality of the doc_content_chars_max parameter.

Fixes #6027 (issue)

@dean-boi
Copy link

Noobie question perhaps: why has this got a limit at all? Should it not default to type None?

@hp0404
Copy link
Contributor Author

hp0404 commented Jun 13, 2023

Noobie question perhaps: why has this got a limit at all? Should it not default to type None?

It seems that @dev2049 introduced the doc_content_chars_max parameter with the default 4000 chars value in this PR - I'm not sure why it's 4000 and not some other value (or None), but we can change it too in this PR if needed.

@dean-boi
Copy link

Hi again, I made some further changes to the arxiv document loader. I'm not sure what the proceedure is though; should I submit a pull request to your branch [hp0404:arxiv-doc-content-chars] or to [hwchase17:master]?

@hp0404
Copy link
Contributor Author

hp0404 commented Jun 13, 2023

Hi again, I made some further changes to the arxiv document loader. I'm not sure what the proceedure is though; should I submit a pull request to your branch [hp0404:arxiv-doc-content-chars] or to [hwchase17:master]?

The best practice in this situation would depend on the development workflow and the specific guidelines the project maintainers set- let's wait for them to respond because I'm not sure as well... e.g.,

  • you can submit the pull request to the branch "hp0404:arxiv-doc-content-chars"; this PR gets updated with your commits and possibly merged into the master as a single PR.
  • or you could create another/alternative PR directly to the hwchase17:master branch, ignoring this one altogether (e.g. I only fixed the issue related to ArxivAPIWrapper as noted in the original issue while it seems you also updated the Arxiv document loader - should it be a separate PR or should this PR be broader and include both ArxivAPIWrapper and Arxiv document loader updates?)
  • or wait for this PR to be merged and propose additional changes in a separate PR to the updated hwchase17:master branch (that will include this PR)
  • etc.

@hwchase17 hwchase17 merged commit b01cf0d into langchain-ai:master Jun 16, 2023
This was referenced Jun 25, 2023
@dosubot dosubot bot mentioned this pull request Oct 11, 2023
14 tasks
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.

ArxivAPIWrapper
3 participants