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

Fix: Resolve pytype --strict-none-binding issue in the api client #3214

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jkppr
Copy link
Collaborator

@jkppr jkppr commented Oct 21, 2024

This PR addresses pytype errors raised by the --strict-none-binding flag in the api client Search class. The issue stemmed from self._raw_response being initialized to None and then populated later by _execute_query, leading to potential unsoundness when accessing it directly.

The fix modifies _execute_query to always return a value: a dictionary with the search results if the results are not saved to a file and count=False, the total count if count=True, or None if the results are saved to a file. The to_dict method now calls _execute_query if self._raw_response is None and handles the case where _execute_query returns None (indicating a previous to_file call) by raising a ValueError. This approach avoids the less robust solution of using assertions and ensures that to_pandas and other methods always have a valid dictionary to work with.

A similar issue was fixed in story.py to handle cases where a view block did not have a populated view attribute, preventing a potential error.

These changes improve type safety and robustness in the Search and Story classes.

@jkppr jkppr requested a review from tomchop October 21, 2024 16:10
@jkppr jkppr self-assigned this Oct 21, 2024
Comment on lines +1087 to +1088
if not self._raw_response:
raise ValueError("No results to return.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

If _raw_response contains an empty list (no results) will this still evaluate to True? Why not re-use the is None clause instead? Same comment for L 1113

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants