-
Notifications
You must be signed in to change notification settings - Fork 20
[breaking] add argument and return value type-hints #187
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
Conversation
brettmc
commented
Mar 27, 2025
- add types::ArgumentTypeHint and types::ReturnTypeHint
- create interfaces before classes, so that classes can implement interfaces provided by module
- [breaking] ReturnType accepts a ReturnTypeHint instead of a TypeInfo, change argument name from type_info to type_hint
- add API to allow args and return types to be nullable: Argument.allow_null()
- add API to allow arguments to have a default value: Argument.with_default_value(CString)
- tests + documentation updates
registering interfaces before classes allows classes to implement interfaces
4d9020f
to
bfa9e1d
Compare
|
||
// leak Interface so that ClassEntry can be retrieved later, during module | ||
// startup | ||
let i_foo_copy: &'static Interface = Box::leak(Box::new(i_foo)); |
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.
I am not happy with this, it's ugly. I wonder whether the whole late-loading-via-functions issue could be removed if we just accepted a String for interface name, and looked it up from globals when needed?
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.
Actually, you can find the interface through this method: https://docs.rs/phper/latest/phper/classes/struct.ClassEntry.html#method.from_globals
I think we can modify the prototype of implements
, something like:
fn implements(&mut self, interface: Interface)
Then we can think about how to design it better later.
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.
Created an issue to address this in a separate PR
I will tackle the <= 8.1 build failures tomorrow. |
Thank you for your contribution. |
} | ||
|
||
/// Argument is optional (also see by_<ref|val>_optional) | ||
pub fn optional(mut self) -> Self { |
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.
I think since this is a Breaking Change, we could remove by_ref_optional
and by_val_optional
and add by_ref()
and by_val
setters. So something like Argument::new('name').by_ref().optional()
, with pass_by_ref:false being the default ?
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.
Allright!
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.
Do you want to make this change in this PR?
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.
Done!
use the Builder pattern to create an Argument or ReturnType, using methods to change the defaults. A default is to be required, pass-by-val, not-nullable