Skip to content

Conversation

dandavison
Copy link
Contributor

Miscellaneous type error fixes, making progress towards fixing them all, opening PR for this subset since this is how many got done right now and we don't want them to pick up conflicts.

Reviewers: the commit named "interesting" contains slightly more interesting fixes.

@dandavison dandavison requested a review from a team as a code owner July 17, 2025 04:25
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Nothing blocking



class MetricCounter(MetricCommon):
class MetricCounter(MetricCommon, ABC):
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting. Is re-inheriting ABC at every level a general Python preference or just a preference of a certain tool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pyright was telling us to do it since they were effectively still ABCs

https://docs.basedpyright.com/v1.31.0/configuration/config-files/#reportImplicitAbstractClass

We can disable any we don't like, but I was thinking that we should err on the side of going with them.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, actually that's basedpyright-specific. I'm going to revert it.

Comment on lines -130 to +131
**_WorkflowExternFunctions(
__temporal_opentelemetry_completed_span=self._completed_workflow_span,
)
{
"__temporal_opentelemetry_completed_span": self._completed_workflow_span,
}
Copy link
Member

Choose a reason for hiding this comment

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

What is the harm in using the constructor form of the TypedDict here instead of the dict literal with string literals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't see how to make it work with pyright (and your comment already indicated mypy probelems)

$ uv run pyright
/Users/dan/src/temporalio/sdk-python/temporalio/contrib/opentelemetry.py
  /Users/dan/src/temporalio/sdk-python/temporalio/contrib/opentelemetry.py:130:15 - error: Argument of type "object" cannot be assigned to parameter "kwargs" of type "(...) -> Unknown" in function "update"
    Type "object" is not assignable to type "(...) -> Unknown" (reportArgumentType)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, unfortunate that we can't use that nice constructor-like kwarg approach afforded to us by typed dicts

@dandavison dandavison force-pushed the dan-0004-no-more-type-errors branch 2 times, most recently from 382ee2c to 621525c Compare July 19, 2025 16:50
Paramaterize OpenAI agents with context type in test
@dandavison dandavison force-pushed the dan-0004-no-more-type-errors branch from 621525c to 471f57b Compare July 22, 2025 01:41
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Test Validation Silently Ignores Unexpected Errors

The test validation function no longer fails on unexpected type errors. A removed loop previously ensured tests failed if any type errors occurred beyond those explicitly expected via assertion comments. Now, such unexpected errors are silently ignored, reducing test strictness and potentially masking new type issues or regressions.

tests/test_type_errors.py#L84-L90

)
def _has_type_error_assertions(test_file: Path) -> bool:
"""Check if a file contains any type error assertions."""
with open(test_file) as f:
return any(re.search(r"# assert-type-error-\w+:", line) for line in f)

Fix in CursorFix in Web


Was this report helpful? Give feedback by reacting with 👍 or 👎

@dandavison
Copy link
Contributor Author

Bug: Test Validation Silently Ignores Unexpected Errors

The test validation function no longer fails on unexpected type errors. A removed loop previously ensured tests failed if any type errors occurred beyond those explicitly expected via assertion comments. Now, such unexpected errors are silently ignored, reducing test strictness and potentially masking new type issues or regressions.

I removed this assertion because it was failing on type: ignored code. It wasn't appropriate to make those drive-by assertions in the course of checking the type error assertions. Checking for type errors in general is done by the type checkers which, unlike the type error test, are running with the type: ignores enabled.

@dandavison dandavison merged commit 7c57a76 into main Jul 22, 2025
17 checks passed
@dandavison dandavison deleted the dan-0004-no-more-type-errors branch July 22, 2025 03:30
tconley1428 pushed a commit that referenced this pull request Aug 28, 2025
* Fix type errors

* Do not make drive-by check for other type errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants