Skip to content

Conversation

jamesbraza
Copy link
Collaborator

@jamesbraza jamesbraza commented Jul 22, 2025

If you passed a custom timeout to S2's _s2_get_with_retrying, you would hit:

TypeError: paperqa.utils._get_with_retrying() got multiple values for keyword argument 'timeout'

This PR fixes that, and allows passing zero timeout or empty headers

@jamesbraza jamesbraza self-assigned this Jul 22, 2025
@Copilot Copilot AI review requested due to automatic review settings July 22, 2025 19:27
@jamesbraza jamesbraza added the bug Something isn't working label Jul 22, 2025
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jul 22, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a TypeError in the _s2_get_with_retrying function where passing a custom timeout parameter would cause a "multiple values for keyword argument" error. The fix changes how keyword arguments are extracted from get_kwargs to prevent conflicts.

  • Changed get_kwargs.get() to get_kwargs.pop() for both headers and timeout parameters
  • Ensures keyword arguments are properly consumed and not passed twice to the underlying function

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Regression: Falsy Values No Longer Trigger Defaults

The change from get_kwargs.get(key) or default to get_kwargs.pop(key, default) for headers and timeout parameters introduces a regression. Previously, falsy values (e.g., None, {}, "") would trigger a fallback to default values. Now, these falsy values are passed directly, breaking existing behavior and potentially causing HTTP request failures or unexpected behavior in _get_with_retrying. The original fallback for falsy values must be restored.

paperqa/clients/semantic_scholar.py#L124-L129

url=url,
headers=get_kwargs.pop("headers", semantic_scholar_headers()),
timeout=(
get_kwargs.pop(
"timeout", aiohttp.ClientTimeout(SEMANTIC_SCHOLAR_API_REQUEST_TIMEOUT)
)

Fix in CursorFix in Web


Comment bugbot run to trigger another review on this PR
Was this report helpful? Give feedback by reacting with 👍 or 👎

@jamesbraza jamesbraza merged commit 8d38006 into main Jul 23, 2025
5 checks passed
@jamesbraza jamesbraza deleted the better-s2-reuse branch July 23, 2025 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant