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

Mark textmap.Getter and textmap.Setter Generic #2656

Closed
alanisaac opened this issue May 2, 2022 · 2 comments · Fixed by #2657
Closed

Mark textmap.Getter and textmap.Setter Generic #2656

alanisaac opened this issue May 2, 2022 · 2 comments · Fixed by #2657
Assignees

Comments

@alanisaac
Copy link
Contributor

alanisaac commented May 2, 2022

Is your feature request related to a problem?
Currently, if I inherit a custom textmap.Getter or textmap.Setter, I can't properly type hint it. For example, take this class:

from opentelemetry.propagators.textmap import Getter
from typing import Dict, Optional, List

class SimpleGetter(Getter):
    def get(
        self, carrier: Dict[str, List[str]], key: str
    ) -> Optional[List[str]]:
        return carrier[key]

When checked with mypy==0.920:

main.py:6: error: Argument 1 of "get" is incompatible with supertype "Getter"; supertype defines the argument type as "CarrierT"
main.py:6: note: This violates the Liskov substitution principle
main.py:6: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

If I try to type hint carrier with the CarrierT TypeVar:

main.py:9: error: Value of type "CarrierT" is not indexable
Found 1 error in 1 file (checked 1 source file)

Describe the solution you'd like
Could Getter and Setter should be marked / inherit from Generic[CarrierT], so the specific TCarrier type can be defined? Happy to contribute a PR.

Describe alternatives you've considered
N/A

Additional context

Note: I see from #1690 that this could be somewhat more complicated. Specifically #1690 (comment)

@aabmass
Copy link
Member

aabmass commented May 3, 2022

Additional context

Note: I see from #1690 that this could be somewhat more complicated. Specifically #1690 (comment)

Thanks for digging into the previous discussions 🙂 it was difficult but might be worth giving it another shot. I would be happy to review a PR for this!

@alanisaac
Copy link
Contributor Author

@aabmass see #2657

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

Successfully merging a pull request may close this issue.

2 participants