-
Notifications
You must be signed in to change notification settings - Fork 23
Improve type error messages to show types instead of values #226
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
base: master
Are you sure you want to change the base?
Conversation
- Modified assertion error messages to include expected Python type - Changed overload error to show argument types instead of values - Added function name to overload error messages for better context Example improvements: - Before: 'arg my_arg wrong type' - After: 'arg my_arg wrong type (expected list[int])' - Before: 'can not handle type of (value1, value2)' - After: 'can not handle types (str, list) as arguments for function myFunc()'
📝 WalkthroughWalkthroughEnhanced error reporting in CodeGenerator.py by propagating expected Python typing information through type checks and assertion generation. Improved assertion and overload error messages to include expected type information and actual argument types for clearer diagnostics. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
autowrap/CodeGenerator.py (1)
1208-1209: Improved assertion message is helpful; minor edge case consideration.The addition of expected type information in the assertion message is a solid improvement for debugging.
One minor edge case: if
expected_typeever contains a single quote character, the generated assertion string would be malformed. While standard Python type annotations don't include quotes, you could add a simple escape:🔎 Optional defensive fix
for n, check, expected_type in checks: - code.add(" assert %s, 'arg %s wrong type (expected %s)'" % (check, n, expected_type)) + code.add(" assert %s, 'arg %s wrong type (expected %s)'" % (check, n, str(expected_type).replace("'", "\\'")))This is a low-risk edge case, so this suggestion is optional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
autowrap/CodeGenerator.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.1.0, 3.12)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.1.0, 3.13)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.1.0, 3.11)
🔇 Additional comments (2)
autowrap/CodeGenerator.py (2)
991-996: Improved error message looks good.The change correctly shows argument types instead of values and includes the function name for better diagnostics. The generated runtime code will properly format the type names.
1130-1130: Clean propagation of type information.The addition of
py_typing_typeto the checks tuple correctly supports the improved assertion messages downstream.
|
Hmm not sure anymore. I have the feeling all these type exception are super unpythonic. Maybe we should just remove them.. |
|
hmm no strong opinion. how would it fail without them? |
|
I honestly think you would get similar TypeError exceptions from cython directly. The types might be called slightly differently (maybe you get errors about cython or python c API level) and cython does not check nested types of course. But I have never tried. Note that for overloaded functions it gets even more tricky to check all possible combinations but there we need to do some kind of check to choose the correct overload anyway. It's probably even too simple what we are doing right now for really complex cases. Idk what's best. |
Example improvements:
Before: 'arg my_arg wrong type'
After: 'arg my_arg wrong type (expected list[int])'
Before: 'can not handle type of (value1, value2)'
After: 'can not handle types (str, list) as arguments for function myFunc()'
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.