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

Improve default values for str, numbers and bool in text_signature #3050

Merged
merged 1 commit into from Mar 25, 2023
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions guide/src/function/signature.md
Expand Up @@ -304,7 +304,7 @@ fn add(a: u64, b: u64) -> u64 {
# .extract()?;
#
# #[cfg(Py_3_8)] // on 3.7 the signature doesn't render b, upstream bug?
# assert_eq!(sig, "(a, b=Ellipsis, /)");
# assert_eq!(sig, "(a, b=0, /)");
#
# Ok(())
# })
Expand All @@ -317,7 +317,7 @@ The following IPython output demonstrates how this generated signature will be s
>>> pyo3_test.add.__text_signature__
'(a, b=..., /)'
>>> pyo3_test.add?
Signature: pyo3_test.add(a, b=Ellipsis, /)
Signature: pyo3_test.add(a, b=0, /)
Docstring: This function adds two unsigned 64-bit integers.
Type: builtin_function_or_method
```
Expand Down
1 change: 1 addition & 0 deletions newsfragments/3050.changed.md
@@ -0,0 +1 @@
Improve default values for str, numbers and bool in automatically-generated `text_signature`.
31 changes: 27 additions & 4 deletions pyo3-macros-backend/src/pyfunction/signature.rs
Expand Up @@ -585,6 +585,29 @@ impl<'a> FunctionSignature<'a> {
}
}

fn default_value_for_parameter(&self, parameter: &str) -> String {
let mut default = "...".to_string();
if let Some(fn_arg) = self.arguments.iter().find(|arg| arg.name == parameter) {
if let Some(syn::Expr::Lit(syn::ExprLit { lit, .. })) = fn_arg.default.as_ref() {
match lit {
syn::Lit::Str(s) => default = s.token().to_string(),
syn::Lit::Char(c) => default = c.token().to_string(),
syn::Lit::Int(i) => default = i.base10_digits().to_string(),
syn::Lit::Float(f) => default = f.base10_digits().to_string(),
syn::Lit::Bool(b) => {
default = if b.value() {
"True".to_string()
} else {
"False".to_string()
}
}
_ => {}
}
}
}
default
}

pub fn text_signature(&self, fn_type: &FnType) -> String {
// automatic text signature generation
let self_argument = match fn_type {
Expand Down Expand Up @@ -624,8 +647,8 @@ impl<'a> FunctionSignature<'a> {
output.push_str(parameter);

if i >= py_sig.required_positional_parameters {
// has a default, just use ... for now
output.push_str("=...");
output.push('=');
output.push_str(&self.default_value_for_parameter(parameter));
Copy link
Member

Choose a reason for hiding this comment

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

So because self.default_value_for_parameter does a search of the arguments by name, this is O(n²), but only for small n, so it's probably fine.

I was planning to avoid that by storing the default expressions in self.python_signature, but that was harder with the deprecated args attribute still present.

Copy link
Member Author

Choose a reason for hiding this comment

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

So because self.default_value_for_parameter does a search of the arguments by name, this is O(n²), but only for small n, so it's probably fine.

Yeah, I think the same.

}

if py_sig.positional_only_parameters > 0 && i + 1 == py_sig.positional_only_parameters {
Expand All @@ -646,8 +669,8 @@ impl<'a> FunctionSignature<'a> {
maybe_push_comma(&mut output);
output.push_str(parameter);
if !required {
// has a default, just use ... for now
output.push_str("=...")
output.push('=');
output.push_str(&self.default_value_for_parameter(parameter));
}
}

Expand Down
20 changes: 16 additions & 4 deletions tests/test_text_signature.rs
Expand Up @@ -147,6 +147,11 @@ fn test_auto_test_signature_function() {
let _ = (a, b, args, c, d, kwargs);
}

#[pyfunction(signature = (a = 1, /, b = None, c = 1.5, d=5, e = "pyo3", f = 'f', h = true))]
fn my_function_5(a: i32, b: Option<i32>, c: f32, d: i32, e: &str, f: char, h: bool) {
let _ = (a, b, c, d, e, f, h);
}

Python::with_gil(|py| {
let f = wrap_pyfunction!(my_function)(py).unwrap();
py_assert!(py, f, "f.__text_signature__ == '(a, b, c)'");
Expand All @@ -155,13 +160,20 @@ fn test_auto_test_signature_function() {
py_assert!(py, f, "f.__text_signature__ == '($module, a, b, c)'");

let f = wrap_pyfunction!(my_function_3)(py).unwrap();
py_assert!(py, f, "f.__text_signature__ == '(a, /, b=..., *, c=...)'");
py_assert!(py, f, "f.__text_signature__ == '(a, /, b=..., *, c=5)'");

let f = wrap_pyfunction!(my_function_4)(py).unwrap();
py_assert!(
py,
f,
"f.__text_signature__ == '(a, /, b=..., *args, c, d=..., **kwargs)'"
"f.__text_signature__ == '(a, /, b=..., *args, c, d=5, **kwargs)'"
);

let f = wrap_pyfunction!(my_function_5)(py).unwrap();
py_assert!(
py,
f,
"f.__text_signature__ == '(a=1, /, b=..., c=1.5, d=5, e=\"pyo3\", f=\\'f\\', h=True)', f.__text_signature__"
);
});
}
Expand Down Expand Up @@ -216,12 +228,12 @@ fn test_auto_test_signature_method() {
py_assert!(
py,
cls,
"cls.method_2.__text_signature__ == '($self, a, /, b=..., *, c=...)'"
"cls.method_2.__text_signature__ == '($self, a, /, b=..., *, c=5)'"
);
py_assert!(
py,
cls,
"cls.method_3.__text_signature__ == '($self, a, /, b=..., *args, c, d=..., **kwargs)'"
"cls.method_3.__text_signature__ == '($self, a, /, b=..., *args, c, d=5, **kwargs)'"
);
py_assert!(
py,
Expand Down