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.

Sign up for GitHub

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

Jump to bottom

Add type hints to dbapi #3068

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Add type hints to dbapi #3068

wants to merge 17 commits into from
+136 −115

Conversation

Kludex
Copy link
Contributor

@Kludex Kludex commented Dec 4, 2024 •
edited
Loading

There are runtime changes i.e. I took out the proxy classes from the inside of the function. This is necessary to have correct type hints.

@xrmx xrmx added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 5, 2024
@Kludex
Copy link
Contributor Author

Kludex commented Dec 10, 2024

Found the issue: https://wrapt.readthedocs.io/en/latest/wrappers.html#proxy-object-attributes

@Kludex
Copy link
Contributor Author

Kludex commented Dec 10, 2024 •
edited
Loading

What's this docs pipeline failing?

@Kludex Kludex mentioned this pull request Dec 10, 2024
Open
emdneto
emdneto reviewed Dec 10, 2024
View reviewed changes
Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

I think to fix docs CI you need to append the class opentelemetry.instrumentation.dbapi.TracedConnectionProxy in nitpick-exceptions.ini file.

@Kludex
Copy link
Contributor Author

Kludex commented Dec 11, 2024

I think to fix docs CI you need to append the class opentelemetry.instrumentation.dbapi.TracedConnectionProxy in nitpick-exceptions.ini file.

What's that nitpick... for? I don't want this class to be documented. 🤔

@Kludex Kludex requested a review from emdneto December 11, 2024 12:23
@Kludex Kludex requested a review from a team as a code owner December 11, 2024 19:01
@Kludex Kludex force-pushed the add-type-hints-to-dbapi branch from 81cd291 to 4171bca Compare December 11, 2024 19:09
@Kludex
Copy link
Contributor Author

Kludex commented Dec 11, 2024

@xrmx @emdneto 👀 👍

emdneto
emdneto approved these changes Dec 13, 2024
View reviewed changes
Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

Thanks! I think would be nice a changelog in that case although there are no overall behavior break. QQ: Do we need py.typed here as well?

@Kludex
Copy link
Contributor Author

Kludex commented Dec 13, 2024

Thanks! I think would be nice a changelog in that case although there are no overall behavior break. QQ: Do we need py.typed here as well?

Yeah, makes sense. I've added the changelog entry more aligned with the fact that I moved the Traced classes to the module level.

@Kludex Kludex requested a review from emdneto December 13, 2024 13:17
@Kludex Kludex mentioned this pull request Dec 20, 2024
Open
@Kludex Kludex mentioned this pull request Dec 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emdneto emdneto emdneto approved these changes

Assignees
No one assigned
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Kludex @emdneto @xrmx