Skip to content

Comments

Improve error when raw2derived argument has no type#1135

Merged
Villtord merged 28 commits intomainfrom
improve-error-when-raw2derived-has-no-type
Dec 19, 2025
Merged

Improve error when raw2derived argument has no type#1135
Villtord merged 28 commits intomainfrom
improve-error-when-raw2derived-has-no-type

Conversation

@Villtord
Copy link
Contributor

@Villtord Villtord commented Nov 17, 2025

Closes #1108

@Villtord Villtord changed the title Improve error when raw2derived has no type Improve error when raw2derived argument has no type Nov 17, 2025
@Villtord Villtord marked this pull request as ready for review November 17, 2025 15:21
@Villtord Villtord self-assigned this Nov 17, 2025
Copy link
Collaborator

@coretl coretl left a comment

Choose a reason for hiding this comment

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

Please could I also have a similar test in test_multi_derived_signal where there are no type hints?

@Villtord
Copy link
Contributor Author

Please could I also have a similar test in test_multi_derived_signal where there are no type hints?

I don't follow - if I add it there it will still test same DerivedSignalFactory which is already tested in test_single_derived

@coretl
Copy link
Collaborator

coretl commented Dec 12, 2025

Please could I also have a similar test in test_multi_derived_signal where there are no type hints?

I don't follow - if I add it there it will still test same DerivedSignalFactory which is already tested in test_single_derived

Yes, but this captures the use case (need to check we get errors when invoked via DerivedSignalFactory as well as via derived_signal_r) rather than the implementation (they both use DerivedSignalFactory under the hood)

Also, would you mind if I let some AI loose on a code review of this PR?

@Villtord
Copy link
Contributor Author

yay AI review! go for it.
So do you want to add in multi_derived_signal a new test that would use some child class of Transform with missing type hints for parameters in raw_to_derived() method?

@coretl
Copy link
Collaborator

coretl commented Dec 12, 2025

So do you want to add in multi_derived_signal a new test that would use some child class of Transform with missing type hints for parameters in raw_to_derived() method?

yes please

yay AI review! go for it.

I'll trigger it, but I'll review it's review before you take any action on it...

@coretl
Copy link
Collaborator

coretl commented Dec 12, 2025

@claude

@claude
Copy link

claude bot commented Dec 12, 2025

Claude finished @coretl's task in 2m 40s —— View job


Code Review Complete

Summary

I've reviewed PR #1135 which improves error messages when raw_to_derived arguments lack type hints. The implementation is solid overall, with good error handling moved to the factory level as requested.

Key findings:

Strengths:

  • Type hint validation correctly moved to DerivedSignalFactory.__init__ for early error detection
  • Good test coverage for the single-signal convenience function case
  • Clean use of inspect.signature + get_type_hints to detect missing annotations

⚠️ Issues to address:

  1. Missing test coverage: Per @coretl's request in comments, need test in test_multi_derived_signal.py testing DerivedSignalFactory directly (not just derived_signal_r convenience function)

  2. Type hints: dict_wrapper() function lacks type annotations

  3. Minor questions: Should _get_params_types_dict also filter out "cls" for classmethods?

Detailed feedback posted as inline comments on specific lines.


Copy link
Contributor

@oliwenmandiamond oliwenmandiamond left a comment

Choose a reason for hiding this comment

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

This looks to be a big improvement of derived signal logic and error handling, thank you!

I would link my issue so it also closes that and update the PR title to be to make improvements to derived signal as we have moved beyond just fixing an error message now

Copy link
Collaborator

@coretl coretl left a comment

Choose a reason for hiding this comment

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

Almost there

@Villtord Villtord merged commit a221add into main Dec 19, 2025
22 checks passed
@Villtord Villtord deleted the improve-error-when-raw2derived-has-no-type branch December 19, 2025 11:11
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.

Improve error when raw-to-derived argument is declared without explicit type in derived_signal_rw

3 participants