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

Added generic to textmap getter and setter #2657

Merged
merged 8 commits into from
May 27, 2022

Conversation

alanisaac
Copy link
Contributor

@alanisaac alanisaac commented May 3, 2022

Description

Add Generic[CarrierT] to textmap.Getter and textmap.Setter in order to allow inherited classes to be typed properly.

Ultimately, the default getter and setter work with more specific types than CarrierT. That means we need to either work with a complex system of overloads or alternatively lie to the type system at some point.

The original code did this through # type: ignore on the function signatures of inherited classes from Getter and Setter. But that means any inherited classes in a type-checked context need to do the same. This PR just lies to the type system in a different place: instead declaring default_getter and default_setter as typed generically but ignoring the fact that the classes DefaultGetter and DefaultSetter are not.

Fixes #2656

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Ran through tox -e mypy to verify changes locally

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8
  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • No. See Added generic to textmap getter and setter #2657 (comment)

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added (typehints only, N/A)
  • Documentation has been updated (typehints only, N/A)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 3, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: alanisaac / name: Alan Isaac (eff3792)
  • ✅ login: srikanthccv / name: Srikanth Chekuri (362fa9a)

@srikanthccv
Copy link
Member

@alanisaac Is there anything pending or is it ready to be reviewed?

@alanisaac alanisaac marked this pull request as ready for review May 14, 2022 23:28
@alanisaac alanisaac requested a review from a team May 14, 2022 23:28
@alanisaac
Copy link
Contributor Author

alanisaac commented May 14, 2022

@srikanthccv thanks for the ping. I think this is ready for review, though I'm not sure if this requires the public API / changelog items in the checks. Let me know what else I can do.

@alanisaac
Copy link
Contributor Author

I see the public API check says:

The code in this branch adds the following public symbols:

- opentelemetry-api/src/opentelemetry/propagators/textmap.py
	Getter
	Setter
	DefaultGetter
	DefaultSetter

Please make sure that all of them are strictly necessary, if not, please consider prefixing them with an underscore to make them private. After that, please label this PR with "Skip Public API check".

This PR does not introduce these symbols, but does make them Generic. I think it should be safe to label this with "Skip Public API check"?

@alanisaac
Copy link
Contributor Author

I think this should work in the library itself, but as this is my first PR for this repo I'm not sure about how the downstream effects on the contrib repo are handled. I imagine the contrib repo is also type checking, and if so, when integrated, this PR would break those checks as the signatures are slightly different?

I can create a PR there as well if needed.

@srikanthccv srikanthccv added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label May 24, 2022
@srikanthccv
Copy link
Member

There aren't any type checks running on contrib packages.

@srikanthccv srikanthccv merged commit cad776a into open-telemetry:main May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark textmap.Getter and textmap.Setter Generic
3 participants