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

Stop rewriting class names #53

Merged
merged 3 commits into from
Nov 17, 2023
Merged

Stop rewriting class names #53

merged 3 commits into from
Nov 17, 2023

Conversation

hynek
Copy link
Owner

@hynek hynek commented Nov 17, 2023

Fixes #52

Um any idea why this is failing so weirdly on 3.8 and 3.9 @guacs?

________________________________________________________________ test_get_type_hints ________________________________________________________________

    def test_get_type_hints():
        """
        typing.get_type_hints() works on our objects.

        Regression test for #52.
        """
>       typing.get_type_hints(svcs.Registry)

tests/test_typing.py:16:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../.asdf/installs/python/3.9.17/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/typing.py:1459: in get_type_hints
    value = _eval_type(value, base_globals, localns)
../../.asdf/installs/python/3.9.17/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/typing.py:292: in _eval_type
    return t._evaluate(globalns, localns, recursive_guard)
../../.asdf/installs/python/3.9.17/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/typing.py:554: in _evaluate
    eval(self.__forward_code__, globalns, localns),
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   ???
E   TypeError: unsupported operand type(s) for |: 'ABCMeta' and '_SpecialGenericAlias'

Is this something I can do something about or can I gate it behind a version check?

@guacs
Copy link
Contributor

guacs commented Nov 17, 2023

This is because the | syntax for defining unions is only supported from 3.10 onwards. You have two options:

  • make all the annotations in _core.py use 3.8 compatible annotations like Union[str, int] instaed int | str
  • only run the tests if python version >= 3.10

If you go with the second option, any user that is depending on get_type_hints(Registry) will have to upgrade to 3.10 in order to be able to use it. If you go with the first option, then there shouldn't be any problems regardless of the Python version.

If you're going with the first option, then you should also use Dict[str, int] (Dict being imported from typing_extensions or typing) instead of dict[str, int].

Honestly, I'm not sure which way to go. The first option is annoying because you can't use the modern type hinting syntax. Also, while I need it because litestar will internally call get_type_hints, I don't think there would be too many users that will depend on get_type_hints(Registry) etc.

@hynek
Copy link
Owner Author

hynek commented Nov 17, 2023

Oof this is extremely obnoxious. I'm afraid I'm going with option 2 for now, because rewriting all type hints would be a major undertaking I just don't have the energy for now. 😳

@hynek hynek merged commit 4b05ab8 into main Nov 17, 2023
13 checks passed
@hynek hynek deleted the get-type-hints branch November 17, 2023 06:44
@hynek
Copy link
Owner Author

hynek commented Nov 17, 2023

I hope this won't stop you from writing that litestar integration – I find that very exciting. :|

@guacs
Copy link
Contributor

guacs commented Nov 17, 2023

Oof this is extremely obnoxious. I'm afraid I'm going with option 2 for now, because rewriting all type hints would be a major undertaking I just don't have the energy for now. 😳

I think this is completely fine. If you're willing, I'm happy to raise a PR for rewriting the type hints, but if that's something you don't wanna do right now I completely understand.

@guacs
Copy link
Contributor

guacs commented Nov 17, 2023

I hope this won't stop you from writing that litestar integration – I find that very exciting. :|

Nope :) I should be able to find a way around it and make the necessary changes in litestar itself if needed.

@hynek
Copy link
Owner Author

hynek commented Nov 17, 2023

Cool, keep me posted if you need anything!

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.

get_type_hints fails on any of the svcs types
2 participants