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

Fix typing in ReadableSpan #3528

Merged
merged 10 commits into from
Nov 16, 2023
Merged

Conversation

adriangb
Copy link
Contributor

Description

This fixes some incorrect typing in ReadableSpan, including assumptions about the type of attributes which causes a bug when they're not a BoundedAttributes instance.

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

How Has This Been Tested?

This is a refactor so I'm relying on the existing tests

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@adriangb adriangb requested a review from a team November 15, 2023 04:58
@adriangb
Copy link
Contributor Author

There's a lot more of this work that needs to be done across all of the repo(s). Is there any appetite for this from the maintainer team?

@adriangb
Copy link
Contributor Author

Also I'd skip this from the changelog if possible since it's (mostly) an internal refactor

@srikanthccv srikanthccv added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Nov 15, 2023
@srikanthccv
Copy link
Member

Only the opentelemetry-api package is in good shape and has CI checks and in the rest of the packages, you see half-complete work. We definitely want to see more parts of the codebase especially public API typed.

@aabmass
Copy link
Member

aabmass commented Nov 15, 2023

+1 to that, I'd love to have typing working on the SDK. This is tracked in #1608

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit on the PR title, seems like we're simplifying some things as well not just updating the type annotations

@adriangb
Copy link
Contributor Author

@srikanthccv this is ready for review

@ocelotl ocelotl merged commit 3d9de97 into open-telemetry:main Nov 16, 2023
110 checks passed
@adriangb adriangb deleted the fix-readablespan branch November 16, 2023 20:29
@adriangb
Copy link
Contributor Author

Thanks folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants