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

Detect missing typehint early #120

Merged
merged 2 commits into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/sciline/_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def deduce_key(self) -> Any:
"""Attempt to determine the key (return type) of the provider."""
if (key := get_type_hints(self._func).get('return')) is None:
raise ValueError(
f'Provider {self} lacks type-hint for return value or returns NOne.'
f'Provider {self} lacks type-hint for return value or returns None.'
)
return key

Expand Down Expand Up @@ -175,8 +175,13 @@ def from_function(cls, provider: ToProvider) -> ArgSpec:
"""Parse the argument spec of a provider."""
hints = get_type_hints(provider)
signature = inspect.getfullargspec(provider)
args = {name: hints[name] for name in signature.args}
kwargs = {name: hints[name] for name in signature.kwonlyargs}
try:
args = {name: hints[name] for name in signature.args}
kwargs = {name: hints[name] for name in signature.kwonlyargs}
Comment on lines -178 to +180
Copy link
Member

Choose a reason for hiding this comment

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

So why did this not raise before? This looks the same to me (apart from a better exception message)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. When I initially said

It pushes the detection forward to pipeline.get. But not to the constructor.

I hadn't implemented the Provider class yet. But since I implemented it, the missing type hint was already detected during Pipeline construction.
So yes, this PR only improves the error message.

except KeyError:
raise ValueError(
f'Provider {provider} lacks type-hint for arguments.'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f'Provider {provider} lacks type-hint for arguments.'
f'Provider {provider} lacks type-hint for arguments or return value.'

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that is handled separately

) from None
return cls(args=args, kwargs=kwargs)

@classmethod
Expand Down
16 changes: 16 additions & 0 deletions tests/pipeline_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1250,3 +1250,19 @@ def func(*, a: G[A]) -> int:

pipeline = sl.Pipeline([func], params={G[A]: 3, G[B]: 4})
assert pipeline.compute(int, scheduler=scheduler) == -12


def test_pipeline_detect_missing_argument_typehint() -> None:
def f(x) -> int: # type: ignore[no-untyped-def]
return x # type:ignore[no-any-return]

with pytest.raises(ValueError, match='type-hint'):
sl.Pipeline([f])


def test_pipeline_detect_missing_return_typehint() -> None:
def f(x: int): # type: ignore[no-untyped-def]
return x

with pytest.raises(ValueError, match='type-hint'):
sl.Pipeline([f])
Loading