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

Add type hints to SQLite3 #3057

Merged
merged 7 commits into from
Dec 4, 2024
Merged

Conversation

Kludex
Copy link
Contributor

@Kludex Kludex commented Nov 29, 2024

Let's try again.

Taking the opportunity to open this PR while working on pydantic/logfire#634.

cc @xrmx @emdneto

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

xrmx commented Dec 2, 2024

@Kludex CI is red, you have to use Tuple from typing I guess

@Kludex
Copy link
Contributor Author

Kludex commented Dec 2, 2024

@Kludex CI is red, you have to use Tuple from typing I guess

When you use from __future__ import annotations, you can do whatever you want on the types part because they are not evaluated at runtime.

The problem was the | (pipe) on the bound=. That's used at runtime, and it's only available after 3.10 (or 3.9, I don't remember).

@xrmx
Copy link
Contributor

xrmx commented Dec 2, 2024

@Kludex CI is red, you have to use Tuple from typing I guess

When you use from __future__ import annotations, you can do whatever you want on the types part because they are not evaluated at runtime.

The problem was the | (pipe) on the bound=. That's used at runtime, and it's only available after 3.10 (or 3.9, I don't remember).

Not sure about that:

==================================== ERRORS ====================================
_ ERROR collecting instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py _
instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py:19: in <module>
    from opentelemetry.instrumentation.sqlite3 import SQLite3Instrumentor
instrumentation/opentelemetry-instrumentation-sqlite3/src/opentelemetry/instrumentation/sqlite3/__init__.py:50: in <module>
    from opentelemetry.instrumentation.sqlite3.package import _instruments
instrumentation/opentelemetry-instrumentation-sqlite3/src/opentelemetry/instrumentation/sqlite3/package.py:16: in <module>
    _instruments: tuple[str, ...] = tuple()
E   TypeError: 'type' object is not subscriptable

@Kludex
Copy link
Contributor Author

Kludex commented Dec 2, 2024

@Kludex CI is red, you have to use Tuple from typing I guess

When you use from __future__ import annotations, you can do whatever you want on the types part because they are not evaluated at runtime.
The problem was the | (pipe) on the bound=. That's used at runtime, and it's only available after 3.10 (or 3.9, I don't remember).

Not sure about that:

==================================== ERRORS ====================================
_ ERROR collecting instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py _
instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py:19: in <module>
    from opentelemetry.instrumentation.sqlite3 import SQLite3Instrumentor
instrumentation/opentelemetry-instrumentation-sqlite3/src/opentelemetry/instrumentation/sqlite3/__init__.py:50: in <module>
    from opentelemetry.instrumentation.sqlite3.package import _instruments
instrumentation/opentelemetry-instrumentation-sqlite3/src/opentelemetry/instrumentation/sqlite3/package.py:16: in <module>
    _instruments: tuple[str, ...] = tuple()
E   TypeError: 'type' object is not subscriptable

I'm missing the future annotations on that file...

…telemetry/instrumentation/sqlite3/package.py
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.

Thanks!

@aabmass aabmass enabled auto-merge (squash) December 3, 2024 20:00
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.

@Kludex any chance you can fix the failing CI?

auto-merge was automatically disabled December 4, 2024 14:06

Head branch was pushed to by a user without write access

@xrmx xrmx enabled auto-merge (squash) December 4, 2024 14:14
@xrmx xrmx merged commit f393546 into open-telemetry:main Dec 4, 2024
573 checks passed
@Kludex Kludex deleted the add-type-hints-to-sqlite3 branch December 4, 2024 14:20
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