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

heroic_util: Remove deprecated type hints #444

Merged

Conversation

sonic2kk
Copy link
Contributor

@sonic2kk sonic2kk commented Aug 12, 2024

This PR removes the deprecated type hints List and Dict imports from typing and replaces them with standard usage of list and dict that are available since Python 3.9. It also introduces some further type hints to other parts of the util file, although incomplete as typing some parts of this is a little complex given our reliance on parsing JSON (ideally we'll eventually have a JSON type alias, and we will more specifically hint in places where we know the structure of the JSON coming back, like if we know certain objects that we're performing .get on are always expected to be dict[str, str] for example). The rationale for further type hinting in the file is to make it easier for unit tests once we eventually get to that stage.

@DavidoTek
Copy link
Owner

Thanks!

although incomplete as typing some parts of this is a little complex given our reliance on parsing JSON (ideally we'll eventually have a JSON type alias, and we will more specifically hint in places where we know the structure of the JSON coming back, like if we know certain objects that we're performing .get on are always expected to be dict[str, str] for example).

If we know that the returned type will be dict[str, str] we can use that. The only problem is that the JSON could potentially change and the data could be dict[str, Any] (or even Any).
Ideally, we would check if the type is correct before returning it. Not sure if it is worth the effort (I would like if Python had something like https://zod.dev integrated... 😄)
-> I think it is okay to type-hint dict[str, str] in the future if we assume that is the expected type.

@DavidoTek DavidoTek merged commit c20b259 into DavidoTek:main Aug 20, 2024
1 check passed
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