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

Add Type hints to function.py #1827

Merged
merged 7 commits into from
May 21, 2024
Merged

Add Type hints to function.py #1827

merged 7 commits into from
May 21, 2024

Conversation

Caiofcas
Copy link
Contributor

Continuation of #1821 (issue #1533 ).

Notable changes:

  • Add _JsonSafeTypes in utils.py in an attempt to properly catch all cases in .to_dict() methods.
  • Same Mapping -> MutableMapping and .copy() -> deepcopy() changes in SF() function as was done in Q().
  • Still having some Any types in kwargs and params, maybe create a new ticket to tighten these up after initial type hints are done? don't know if I can just go by the test cases and assume types not present in them are invalid, the way the functions are written it seems purposefully loose.

Copy link

cla-checker-service bot commented May 17, 2024

💚 CLA has been signed

@miguelgrinberg
Copy link
Collaborator

Add _JsonSafeTypes in utils.py in an attempt to properly catch all cases in .to_dict() methods.

Does this type need the _prefix?

Same Mapping -> MutableMapping and .copy() -> deepcopy() changes in SF() function as was done in Q().

Looks good.

Still having some Any types in kwargs and params, maybe create a new ticket to tighten these up after initial type hints are done? don't know if I can just go by the test cases and assume types not present in them are invalid, the way the functions are written it seems purposefully loose.

Yes, this will have to be treated as a separate change. You are correct, this was designed many years ago with the idea of being completely dynamic. I think in the end what I'm going to end up doing is add the kwargs explicitly in all of these, but let's not worry about that for this effort.

@Caiofcas
Copy link
Contributor Author

Does this type need the _prefix?

It's not required, I put it there from the perspective of it's an internal library helper type and should probably not be used from the outside. Similar to a numpy _ArrayType or other internal types like it.

@miguelgrinberg
Copy link
Collaborator

@Caiofcas Merging this change, but note that I have renamed _JSONSafeTypes to JSONType. There is a bunch of other internal things in utils.py (and many more in other modules) that do not use the _ prefix, so to me it seems more consistent to not use it.

But anyway, you are making great progress! :)

@miguelgrinberg miguelgrinberg merged commit 76a57fd into elastic:main May 21, 2024
16 checks passed
@miguelgrinberg miguelgrinberg added the backport 8.x Backport to 8.x label May 21, 2024
github-actions bot pushed a commit that referenced this pull request May 21, 2024
* feat: add first type annotations

* feat: add _JSONSafeTypes annotation to to_dict methods

* feat: add typing for SF function

* chore: fix linting

* rename _JSONSafeTypes to JSONType

* format code

---------

Co-authored-by: Miguel Grinberg <miguel.grinberg@gmail.com>
(cherry picked from commit 76a57fd)
miguelgrinberg pushed a commit that referenced this pull request May 21, 2024
* feat: add first type annotations

* feat: add _JSONSafeTypes annotation to to_dict methods

* feat: add typing for SF function

* chore: fix linting

* rename _JSONSafeTypes to JSONType

* format code

---------

Co-authored-by: Miguel Grinberg <miguel.grinberg@gmail.com>
(cherry picked from commit 76a57fd)

Co-authored-by: Caio Fontes <38675540+Caiofcas@users.noreply.github.com>
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