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

Fixed issue #92 #95

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Fixed issue #92 #95

merged 2 commits into from
Oct 23, 2024

Conversation

rgeronimi
Copy link
Contributor

@rgeronimi rgeronimi commented Oct 14, 2024

This fixes

#92

Calling nest_asyncio.apply() from this class is useless as it is already called from the base synchronous master object that controls all the API, SemanticScholar.

Successfully tested in both synchronous and asynchronous modes.

@rgeronimi rgeronimi changed the title Fixed issue #92 Fixed issue https://github.com/danielnsilva/semanticscholar/issues/92 Oct 14, 2024
@rgeronimi rgeronimi changed the title Fixed issue https://github.com/danielnsilva/semanticscholar/issues/92 Fixed issue #92 Oct 14, 2024
@danielnsilva
Copy link
Owner

This PR also addresses #93

@rgeronimi
Copy link
Contributor Author

Does it satisfy your quality criteria ? I was very careful with the code changes. Don't hesitate if your have any question.
PS : for the future I think the PaginatedResults class could be split in 2 sync vs async versions. This would make the API and code much clearer than it is today, exactly like the SemanticScholar class.

@danielnsilva
Copy link
Owner

LGTM! Thanks @dmoklaf

@danielnsilva danielnsilva merged commit 4cafc2b into danielnsilva:master Oct 23, 2024
6 checks passed
@danielnsilva
Copy link
Owner

PS : for the future I think the PaginatedResults class could be split in 2 sync vs async versions. This would make the API and code much clearer than it is today, exactly like the SemanticScholar class.

It might be worth opening an issue or PR to discuss this further.

@rgeronimi
Copy link
Contributor Author

Hi, is this possible to release a new version with this? Thx

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.

2 participants