Skip to content

replace JSONType with Any #1856

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

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

miguelgrinberg
Copy link
Collaborator

@miguelgrinberg miguelgrinberg commented Jul 4, 2024

One of the issues I'm facing when trying to type the examples is that JSONType was used in many places to represent responses from the API. Because this is an imperfect type hint, it requires adding casts all over the place.

With this change I'm removing JSONType and replacing it with Any. This matches the commonly used ObjectApiResponse[Any] from the low-level client, and lets mypy digest attribute accesses without generating warnings, in things like self.meta.inner_hits.answer.hits. Just by making this change a few type ignores can be removed, and no new typing errors appeared.

I think in the future I will revisit this and create proper types for the response structure, but for now I think Any is the best choice.

@miguelgrinberg miguelgrinberg requested a review from pquentin July 4, 2024 10:18
@miguelgrinberg miguelgrinberg added the backport 8.x Backport to 8.x label Jul 4, 2024
@miguelgrinberg miguelgrinberg merged commit d045a99 into elastic:main Jul 4, 2024
17 checks passed
@miguelgrinberg miguelgrinberg deleted the jsontype-to-any branch July 4, 2024 10:23
github-actions bot pushed a commit that referenced this pull request Jul 4, 2024
(cherry picked from commit d045a99)
miguelgrinberg added a commit that referenced this pull request Jul 4, 2024
(cherry picked from commit d045a99)

Co-authored-by: Miguel Grinberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 8.x Backport to 8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants