-
Notifications
You must be signed in to change notification settings - Fork 883
fill PyTypeInfo::NAME with Pythonic values (e.g. BaseException) instead of C ones (e.g. PyBaseException) #5352
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: main
Are you sure you want to change the base?
Conversation
pyobject_native_static_type_object!(ffi::PyBaseObject_Type), | ||
Some("builtins"), | ||
"object", | ||
None, |
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.
Not sure if we should set "builtins"
there.
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.
Hmm. It seems sorta useless but maybe could have unexpected effects removing, what was the motivation to remove?
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.
No real motivation at the moment. Added back. See also #5364 for an aside related to this topic.
ede5c0d
to
41a554c
Compare
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.
Thanks, overall makes the error messages look much better IMO and I guess gives us opportunities to clean other stuff up 🤞
pyobject_native_static_type_object!(ffi::PyBaseObject_Type), | ||
Some("builtins"), | ||
"object", | ||
None, |
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.
Hmm. It seems sorta useless but maybe could have unexpected effects removing, what was the motivation to remove?
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.
Thanks, generally I think this looks great. A few questions / final adjustments.
I wonder if, for sake of not breaking users, we should consider deprecating NAME
and just having FULLY_QUALIFIED_NAME
, if we can make that work. Otherwise, I think we'd want to mention this in the migration guide.
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
Yes! I think this MR is still relevant: we will just concatenate the module name and the type name to generate the |
# Conflicts: # src/exceptions.rs # src/types/code.rs # src/types/datetime.rs
No description provided.