diff --git a/examples/pyo3_benchmarks/Cargo.toml b/examples/pyo3_benchmarks/Cargo.toml new file mode 100644 index 00000000..f931e19a --- /dev/null +++ b/examples/pyo3_benchmarks/Cargo.toml @@ -0,0 +1,16 @@ +[package] +authors = ["PyO3 Authors"] +name = "pyo3-benchmarks" +version = "0.1.0" +description = "Python-based benchmarks for various PyO3 functionality" +edition = "2018" + +[dependencies] + +[dependencies.pyo3] +path = "../../" +features = ["extension-module"] + +[lib] +name = "_pyo3_benchmarks" +crate-type = ["cdylib"] diff --git a/examples/pyo3_benchmarks/MANIFEST.in b/examples/pyo3_benchmarks/MANIFEST.in new file mode 100644 index 00000000..becccf7b --- /dev/null +++ b/examples/pyo3_benchmarks/MANIFEST.in @@ -0,0 +1,2 @@ +include pyproject.toml Cargo.toml +recursive-include src * diff --git a/examples/pyo3_benchmarks/README.md b/examples/pyo3_benchmarks/README.md new file mode 100644 index 00000000..bf8867f5 --- /dev/null +++ b/examples/pyo3_benchmarks/README.md @@ -0,0 +1,17 @@ +# rustapi_module + +A simple extension module built using PyO3. + +## Build + +```shell +python setup.py install +``` + +## Testing + +To test install tox globally and run + +```shell +tox -e py +``` diff --git a/examples/pyo3_benchmarks/pyo3_benchmarks/__init__.py b/examples/pyo3_benchmarks/pyo3_benchmarks/__init__.py new file mode 100644 index 00000000..cd0f9b65 --- /dev/null +++ b/examples/pyo3_benchmarks/pyo3_benchmarks/__init__.py @@ -0,0 +1 @@ +from ._pyo3_benchmarks import * diff --git a/examples/pyo3_benchmarks/requirements-dev.txt b/examples/pyo3_benchmarks/requirements-dev.txt new file mode 100644 index 00000000..4dc9c747 --- /dev/null +++ b/examples/pyo3_benchmarks/requirements-dev.txt @@ -0,0 +1,6 @@ +pip>=19.1 +hypothesis>=3.55 +pytest>=3.5.0 +setuptools-rust>=0.10.2 +psutil>=5.6 +pytest-benchmark~=3.2 diff --git a/examples/pyo3_benchmarks/setup.py b/examples/pyo3_benchmarks/setup.py new file mode 100644 index 00000000..0208e369 --- /dev/null +++ b/examples/pyo3_benchmarks/setup.py @@ -0,0 +1,44 @@ +import sys +import platform + +from setuptools import setup +from setuptools_rust import RustExtension + + +def get_py_version_cfgs(): + # For now each Cfg Py_3_X flag is interpreted as "at least 3.X" + version = sys.version_info[0:2] + py3_min = 6 + out_cfg = [] + for minor in range(py3_min, version[1] + 1): + out_cfg.append("--cfg=Py_3_%d" % minor) + + if platform.python_implementation() == "PyPy": + out_cfg.append("--cfg=PyPy") + + return out_cfg + + +setup( + name="pyo3-benchmarks", + version="0.1.0", + classifiers=[ + "License :: OSI Approved :: MIT License", + "Development Status :: 3 - Alpha", + "Intended Audience :: Developers", + "Programming Language :: Python", + "Programming Language :: Rust", + "Operating System :: POSIX", + "Operating System :: MacOS :: MacOS X", + ], + packages=["pyo3_benchmarks"], + rust_extensions=[ + RustExtension( + "pyo3_benchmarks._pyo3_benchmarks", + rustc_flags=get_py_version_cfgs(), + debug=False, + ), + ], + include_package_data=True, + zip_safe=False, +) diff --git a/examples/pyo3_benchmarks/src/lib.rs b/examples/pyo3_benchmarks/src/lib.rs new file mode 100644 index 00000000..e67aaeb8 --- /dev/null +++ b/examples/pyo3_benchmarks/src/lib.rs @@ -0,0 +1,33 @@ +use pyo3::prelude::*; +use pyo3::types::{PyDict, PyTuple}; +use pyo3::wrap_pyfunction; + +#[pyfunction(args = "*", kwargs = "**")] +fn args_and_kwargs<'a>( + args: &'a PyTuple, + kwargs: Option<&'a PyDict>, +) -> (&'a PyTuple, Option<&'a PyDict>) { + (args, kwargs) +} + +#[pyfunction(a, b = 2, args = "*", c = 4, kwargs = "**")] +fn mixed_args<'a>( + a: i32, + b: i32, + args: &'a PyTuple, + c: i32, + kwargs: Option<&'a PyDict>, +) -> (i32, i32, &'a PyTuple, i32, Option<&'a PyDict>) { + (a, b, args, c, kwargs) +} + +#[pyfunction] +fn no_args() {} + +#[pymodule] +fn _pyo3_benchmarks(_py: Python<'_>, m: &PyModule) -> PyResult<()> { + m.add_function(wrap_pyfunction!(args_and_kwargs, m)?)?; + m.add_function(wrap_pyfunction!(mixed_args, m)?)?; + m.add_function(wrap_pyfunction!(no_args, m)?)?; + Ok(()) +} diff --git a/examples/pyo3_benchmarks/tests/test_benchmarks.py b/examples/pyo3_benchmarks/tests/test_benchmarks.py new file mode 100644 index 00000000..f8fd54de --- /dev/null +++ b/examples/pyo3_benchmarks/tests/test_benchmarks.py @@ -0,0 +1,46 @@ +import pyo3_benchmarks + + +def test_args_and_kwargs(benchmark): + benchmark(pyo3_benchmarks.args_and_kwargs, 1, 2, 3, a=4, foo=10) + + +def args_and_kwargs_py(*args, **kwargs): + return (args, kwargs) + + +def test_args_and_kwargs_py(benchmark): + rust = pyo3_benchmarks.args_and_kwargs(1, 2, 3, bar=4, foo=10) + py = args_and_kwargs_py(1, 2, 3, bar=4, foo=10) + assert rust == py + benchmark(args_and_kwargs_py, 1, 2, 3, bar=4, foo=10) + + +def test_mixed_args(benchmark): + benchmark(pyo3_benchmarks.mixed_args, 1, 2, 3, bar=4, foo=10) + + +def mixed_args_py(a, b=2, *args, c=4, **kwargs): + return (a, b, args, c, kwargs) + + +def test_mixed_args_py(benchmark): + rust = pyo3_benchmarks.mixed_args(1, 2, 3, bar=4, foo=10) + py = mixed_args_py(1, 2, 3, bar=4, foo=10) + assert rust == py + benchmark(mixed_args_py, 1, 2, 3, bar=4, foo=10) + + +def test_no_args(benchmark): + benchmark(pyo3_benchmarks.no_args) + + +def no_args_py(): + return None + + +def test_no_args_py(benchmark): + rust = pyo3_benchmarks.no_args() + py = no_args_py() + assert rust == py + benchmark(no_args_py) diff --git a/examples/pyo3_benchmarks/tox.ini b/examples/pyo3_benchmarks/tox.ini new file mode 100644 index 00000000..e0d45bbc --- /dev/null +++ b/examples/pyo3_benchmarks/tox.ini @@ -0,0 +1,10 @@ +[tox] +# can't install from sdist because local pyo3 repo can't be included in the sdist +skipsdist = true + +[testenv] +description = Run the unit tests under {basepython} +deps = -rrequirements-dev.txt +commands = + python setup.py install + pytest {posargs} diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs index d0fac0f1..91fc5df1 100644 --- a/pyo3-macros-backend/src/module.rs +++ b/pyo3-macros-backend/src/module.rs @@ -8,7 +8,7 @@ use crate::pymethod::get_arg_names; use crate::utils; use proc_macro2::{Span, TokenStream}; use quote::{format_ident, quote}; -use syn::{spanned::Spanned, Ident}; +use syn::{spanned::Spanned, Ident, Result}; /// Generates the function that is called by the python interpreter to initialize the native /// module @@ -197,7 +197,7 @@ pub fn add_fn_to_module( let name = &func.sig.ident; let wrapper_ident = format_ident!("__pyo3_raw_{}", name); - let wrapper = function_c_wrapper(name, &wrapper_ident, &spec, pyfn_attrs.pass_module); + let wrapper = function_c_wrapper(name, &wrapper_ident, &spec, pyfn_attrs.pass_module)?; Ok(quote! { #wrapper pub(crate) fn #function_wrapper_ident<'a>( @@ -223,7 +223,7 @@ fn function_c_wrapper( wrapper_ident: &Ident, spec: &method::FnSpec<'_>, pass_module: bool, -) -> TokenStream { +) -> Result { let names: Vec = get_arg_names(&spec); let cb; let slf_module; @@ -240,8 +240,8 @@ fn function_c_wrapper( }; slf_module = None; }; - let body = pymethod::impl_arg_params(spec, None, cb); - quote! { + let body = pymethod::impl_arg_params(spec, None, cb)?; + Ok(quote! { unsafe extern "C" fn #wrapper_ident( _slf: *mut pyo3::ffi::PyObject, _args: *mut pyo3::ffi::PyObject, @@ -256,5 +256,5 @@ fn function_c_wrapper( #body }) } - } + }) } diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 607d9472..8fd59b1a 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -4,8 +4,8 @@ use crate::konst::ConstSpec; use crate::method::{FnArg, FnSpec, FnType, SelfType}; use crate::utils; use proc_macro2::{Span, TokenStream}; -use quote::quote; -use syn::{ext::IdentExt, spanned::Spanned}; +use quote::{quote, quote_spanned}; +use syn::{ext::IdentExt, spanned::Spanned, Result}; pub enum PropertyType<'a> { Descriptor(&'a syn::Field), @@ -22,29 +22,29 @@ pub fn gen_py_method( cls: &syn::Type, sig: &mut syn::Signature, meth_attrs: &mut Vec, -) -> syn::Result { +) -> Result { check_generic(sig)?; let spec = FnSpec::parse(sig, &mut *meth_attrs, true)?; Ok(match &spec.tp { FnType::Fn(self_ty) => GeneratedPyMethod::Method(impl_py_method_def( &spec, - &impl_wrap(cls, &spec, self_ty, true), + &impl_wrap(cls, &spec, self_ty, true)?, )), FnType::FnNew => { - GeneratedPyMethod::New(impl_py_method_def_new(cls, &impl_wrap_new(cls, &spec))) + GeneratedPyMethod::New(impl_py_method_def_new(cls, &impl_wrap_new(cls, &spec)?)) } FnType::FnCall(self_ty) => GeneratedPyMethod::Call(impl_py_method_def_call( cls, - &impl_wrap(cls, &spec, self_ty, false), + &impl_wrap(cls, &spec, self_ty, false)?, )), FnType::FnClass => GeneratedPyMethod::Method(impl_py_method_def_class( &spec, - &impl_wrap_class(cls, &spec), + &impl_wrap_class(cls, &spec)?, )), FnType::FnStatic => GeneratedPyMethod::Method(impl_py_method_def_static( &spec, - &impl_wrap_static(cls, &spec), + &impl_wrap_static(cls, &spec)?, )), FnType::ClassAttribute => GeneratedPyMethod::Method(impl_py_method_class_attribute( &spec, @@ -98,7 +98,7 @@ pub fn impl_wrap( spec: &FnSpec<'_>, self_ty: &SelfType, noargs: bool, -) -> TokenStream { +) -> Result { let body = impl_call(cls, &spec); let slf = self_ty.receiver(cls); impl_wrap_common(cls, spec, noargs, slf, body) @@ -110,10 +110,10 @@ fn impl_wrap_common( noargs: bool, slf: TokenStream, body: TokenStream, -) -> TokenStream { +) -> Result { let python_name = &spec.python_name; if spec.args.is_empty() && noargs { - quote! { + Ok(quote! { unsafe extern "C" fn __wrap( _slf: *mut pyo3::ffi::PyObject, _args: *mut pyo3::ffi::PyObject, @@ -126,11 +126,10 @@ fn impl_wrap_common( pyo3::callback::convert(_py, #body) }) } - } + }) } else { - let body = impl_arg_params(&spec, Some(cls), body); - - quote! { + let body = impl_arg_params(&spec, Some(cls), body)?; + Ok(quote! { unsafe extern "C" fn __wrap( _slf: *mut pyo3::ffi::PyObject, _args: *mut pyo3::ffi::PyObject, @@ -146,18 +145,22 @@ fn impl_wrap_common( pyo3::callback::convert(_py, #body) }) } - } + }) } } /// Generate function wrapper for protocol method (PyCFunction, PyCFunctionWithKeywords) -pub fn impl_proto_wrap(cls: &syn::Type, spec: &FnSpec<'_>, self_ty: &SelfType) -> TokenStream { +pub fn impl_proto_wrap( + cls: &syn::Type, + spec: &FnSpec<'_>, + self_ty: &SelfType, +) -> Result { let python_name = &spec.python_name; let cb = impl_call(cls, &spec); - let body = impl_arg_params(&spec, Some(cls), cb); + let body = impl_arg_params(&spec, Some(cls), cb)?; let slf = self_ty.receiver(cls); - quote! { + Ok(quote! { #[allow(unused_mut)] unsafe extern "C" fn __wrap( _slf: *mut pyo3::ffi::PyObject, @@ -173,27 +176,25 @@ pub fn impl_proto_wrap(cls: &syn::Type, spec: &FnSpec<'_>, self_ty: &SelfType) - pyo3::callback::convert(_py, #body) }) } - } + }) } /// Generate class method wrapper (PyCFunction, PyCFunctionWithKeywords) -pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { +pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> Result { let name = &spec.name; let python_name = &spec.python_name; let names: Vec = get_arg_names(&spec); let cb = quote! { #cls::#name(#(#names),*) }; - let body = impl_arg_params(spec, Some(cls), cb); + let body = impl_arg_params(spec, Some(cls), cb)?; - quote! { + Ok(quote! { #[allow(unused_mut)] unsafe extern "C" fn __wrap( subtype: *mut pyo3::ffi::PyTypeObject, _args: *mut pyo3::ffi::PyObject, _kwargs: *mut pyo3::ffi::PyObject) -> *mut pyo3::ffi::PyObject { - use pyo3::type_object::PyTypeInfo; use pyo3::callback::IntoPyCallbackOutput; - use std::convert::TryFrom; const _LOCATION: &'static str = concat!(stringify!(#cls),".",stringify!(#python_name),"()"); pyo3::callback_body_without_convert!(_py, { @@ -205,19 +206,19 @@ pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { Ok(cell as *mut pyo3::ffi::PyObject) }) } - } + }) } /// Generate class method wrapper (PyCFunction, PyCFunctionWithKeywords) -pub fn impl_wrap_class(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { +pub fn impl_wrap_class(cls: &syn::Type, spec: &FnSpec<'_>) -> Result { let name = &spec.name; let python_name = &spec.python_name; let names: Vec = get_arg_names(&spec); let cb = quote! { #cls::#name(&_cls, #(#names),*) }; - let body = impl_arg_params(spec, Some(cls), cb); + let body = impl_arg_params(spec, Some(cls), cb)?; - quote! { + Ok(quote! { #[allow(unused_mut)] unsafe extern "C" fn __wrap( _cls: *mut pyo3::ffi::PyObject, @@ -233,19 +234,19 @@ pub fn impl_wrap_class(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { pyo3::callback::convert(_py, #body) }) } - } + }) } /// Generate static method wrapper (PyCFunction, PyCFunctionWithKeywords) -pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { +pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> Result { let name = &spec.name; let python_name = &spec.python_name; let names: Vec = get_arg_names(&spec); let cb = quote! { #cls::#name(#(#names),*) }; - let body = impl_arg_params(spec, Some(cls), cb); + let body = impl_arg_params(spec, Some(cls), cb)?; - quote! { + Ok(quote! { #[allow(unused_mut)] unsafe extern "C" fn __wrap( _slf: *mut pyo3::ffi::PyObject, @@ -260,7 +261,7 @@ pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { pyo3::callback::convert(_py, #body) }) } - } + }) } /// Generate a wrapper for initialization of a class attribute from a method @@ -399,11 +400,9 @@ pub fn impl_arg_params( spec: &FnSpec<'_>, self_: Option<&syn::Type>, body: TokenStream, -) -> TokenStream { +) -> Result { if spec.args.is_empty() { - return quote! { - #body - }; + return Ok(body); } let mut positional_parameter_names = Vec::new(); @@ -438,7 +437,7 @@ pub fn impl_arg_params( let mut param_conversion = Vec::new(); let mut option_pos = 0; for (idx, arg) in spec.args.iter().enumerate() { - param_conversion.push(impl_arg_param(&arg, &spec, idx, self_, &mut option_pos)); + param_conversion.push(impl_arg_param(&arg, &spec, idx, self_, &mut option_pos)?); } let (mut accept_args, mut accept_kwargs) = (false, false); @@ -453,25 +452,27 @@ pub fn impl_arg_params( } // create array of arguments, and then parse - quote! {{ - const DESCRIPTION: pyo3::derive_utils::FunctionDescription = pyo3::derive_utils::FunctionDescription { - fname: _LOCATION, - positional_parameter_names: &[#(#positional_parameter_names),*], - // TODO: https://github.com/PyO3/pyo3/issues/1439 - support specifying these - positional_only_parameters: 0, - required_positional_parameters: #required_positional_parameters, - keyword_only_parameters: &[#(#keyword_only_parameters),*], - accept_varargs: #accept_args, - accept_varkeywords: #accept_kwargs, - }; + Ok(quote! { + { + const DESCRIPTION: pyo3::derive_utils::FunctionDescription = pyo3::derive_utils::FunctionDescription { + fname: _LOCATION, + positional_parameter_names: &[#(#positional_parameter_names),*], + // TODO: https://github.com/PyO3/pyo3/issues/1439 - support specifying these + positional_only_parameters: 0, + required_positional_parameters: #required_positional_parameters, + keyword_only_parameters: &[#(#keyword_only_parameters),*], + accept_varargs: #accept_args, + accept_varkeywords: #accept_kwargs, + }; - let mut output = [None; #num_params]; - let (_args, _kwargs) = DESCRIPTION.extract_arguments(_args, _kwargs, &mut output)?; + let mut output = [None; #num_params]; + let (_args, _kwargs) = DESCRIPTION.extract_arguments(_args, _kwargs, &mut output)?; - #(#param_conversion)* + #(#param_conversion)* - #body - }} + #body + } + }) } /// Re option_pos: The option slice doesn't contain the py: Python argument, so the argument @@ -482,13 +483,14 @@ fn impl_arg_param( idx: usize, self_: Option<&syn::Type>, option_pos: &mut usize, -) -> TokenStream { +) -> Result { let arg_name = syn::Ident::new(&format!("arg{}", idx), Span::call_site()); if arg.py { - return quote! { + return Ok(quote_spanned! { + arg.ty.span() => let #arg_name = _py; - }; + }); } let ty = arg.ty; @@ -498,29 +500,26 @@ fn impl_arg_param( }; if spec.is_args(&name) { - return if arg.optional.is_some() { - quote! { - let #arg_name = _args.map(|args| args.extract()) - .transpose() - .map_err(#transform_error)?; - } - } else { - quote! { - let #arg_name = _args.unwrap().extract() - .map_err(#transform_error)?; - } - }; + ensure_spanned!( + arg.optional.is_none(), + arg.name.span() => "args cannot be optional" + ); + return Ok(quote_spanned! { + arg.ty.span() => + let #arg_name = _args.unwrap().extract() + .map_err(#transform_error)?; + }); } else if spec.is_kwargs(&name) { - // FIXME: check the below? - // ensure_spanned!( - // arg.optional.is_some(), - // arg.name.span() => "kwargs must be Option<_>" - // ); - return quote! { + ensure_spanned!( + arg.optional.is_some(), + arg.name.span() => "kwargs must be Option<_>" + ); + return Ok(quote_spanned! { + arg.ty.span() => let #arg_name = _kwargs.map(|kwargs| kwargs.extract()) .transpose() .map_err(#transform_error)?; - }; + }); } let arg_value = quote!(output[#option_pos]); @@ -559,20 +558,22 @@ fn impl_arg_param( ) }; - quote! { + Ok(quote_spanned! { + arg.ty.span() => let #mut_ _tmp: #target_ty = match #arg_value { Some(_obj) => #extract, None => #default, }; let #arg_name = #borrow_tmp; - } + }) } else { - quote! { + Ok(quote_spanned! { + arg.ty.span() => let #arg_name = match #arg_value { Some(_obj) => #extract, None => #default, }; - } + }) }; /// Replace `Self`, remove lifetime and get mutability from the type diff --git a/pyo3-macros-backend/src/pyproto.rs b/pyo3-macros-backend/src/pyproto.rs index f1bbb380..90c3ddee 100644 --- a/pyo3-macros-backend/src/pyproto.rs +++ b/pyo3-macros-backend/src/pyproto.rs @@ -65,7 +65,7 @@ fn impl_proto_impl( let fn_spec = FnSpec::parse(&mut met.sig, &mut met.attrs, false)?; let method = if let FnType::Fn(self_ty) = &fn_spec.tp { - pymethod::impl_proto_wrap(ty, &fn_spec, &self_ty) + pymethod::impl_proto_wrap(ty, &fn_spec, &self_ty)? } else { bail_spanned!( met.sig.span() => "expected method with receiver for #[pyproto] method" diff --git a/src/derive_utils.rs b/src/derive_utils.rs index 4b9b93ba..cb86d721 100644 --- a/src/derive_utils.rs +++ b/src/derive_utils.rs @@ -89,12 +89,12 @@ impl FunctionDescription { // Check that there's sufficient positional arguments once keyword arguments are specified if args_provided < self.required_positional_parameters { - let missing_positional_arguments: Vec<_> = self.positional_parameter_names - [..self.required_positional_parameters] + let missing_positional_arguments: Vec<_> = self + .positional_parameter_names .iter() - .copied() + .take(self.required_positional_parameters) .zip(output.iter()) - .filter_map(|(param, out)| if out.is_none() { Some(param) } else { None }) + .filter_map(|(param, out)| if out.is_none() { Some(*param) } else { None }) .collect(); if !missing_positional_arguments.is_empty() { return Err( @@ -148,33 +148,30 @@ impl FunctionDescription { // Compare the keyword name against each parameter in turn. This is exactly the same method // which CPython uses to map keyword names. Although it's O(num_parameters), the number of // parameters is expected to be small so it's not worth constructing a mapping. - for (param, out) in self.keyword_only_parameters.iter().zip(&mut *kwargs_output) { - if utf8_string == param.name { - *out = Some(value); - continue 'kwarg_loop; - } + if let Some(i) = self + .keyword_only_parameters + .iter() + .position(|param| utf8_string == param.name) + { + kwargs_output[i] = Some(value); + continue 'kwarg_loop; } // Repeat for positional parameters - for (i, (¶m, out)) in self + if let Some((i, param)) = self .positional_parameter_names .iter() - .zip(&mut *args_output) .enumerate() + .find(|&(_, param)| utf8_string == *param) { - if utf8_string == param { - if i < self.positional_only_parameters { - positional_only_keyword_arguments.push(param); - } else { - match out { - Some(_) => return Err(self.multiple_values_for_argument(param)), - None => { - *out = Some(value); - } - } + if i < self.positional_only_parameters { + positional_only_keyword_arguments.push(*param); + } else { + if args_output[i].replace(value).is_some() { + return Err(self.multiple_values_for_argument(param)); } - continue 'kwarg_loop; } + continue 'kwarg_loop; } unexpected_keyword_handler(kwarg_name, value)?; @@ -229,7 +226,7 @@ impl FunctionDescription { "{} got some positional-only arguments passed as keyword arguments: ", self.fname ); - write_parameter_list(&mut msg, parameter_names); + push_parameter_list(&mut msg, parameter_names); PyTypeError::new_err(msg) } @@ -246,7 +243,7 @@ impl FunctionDescription { argument_type, arguments, ); - write_parameter_list(&mut msg, parameter_names); + push_parameter_list(&mut msg, parameter_names); PyTypeError::new_err(msg) } } @@ -392,14 +389,18 @@ impl<'a> From<&'a PyModule> for PyFunctionArguments<'a> { } } -fn write_parameter_list(msg: &mut String, parameter_names: &[&str]) { +fn push_parameter_list(msg: &mut String, parameter_names: &[&str]) { for (i, parameter) in parameter_names.iter().enumerate() { - if i != 0 && parameter_names.len() > 2 { - msg.push(','); - } + if i != 0 { + if parameter_names.len() > 2 { + msg.push(','); + } - if i == parameter_names.len() - 1 { - msg.push_str(" and ") + if i == parameter_names.len() - 1 { + msg.push_str(" and ") + } else { + msg.push(' ') + } } msg.push('\''); @@ -407,3 +408,43 @@ fn write_parameter_list(msg: &mut String, parameter_names: &[&str]) { msg.push('\''); } } + +#[cfg(test)] +mod tests { + use super::push_parameter_list; + + #[test] + fn push_parameter_list_empty() { + let mut s = String::new(); + push_parameter_list(&mut s, &[]); + assert_eq!(&s, ""); + } + + #[test] + fn push_parameter_list_one() { + let mut s = String::new(); + push_parameter_list(&mut s, &["a"]); + assert_eq!(&s, "'a'"); + } + + #[test] + fn push_parameter_list_two() { + let mut s = String::new(); + push_parameter_list(&mut s, &["a", "b"]); + assert_eq!(&s, "'a' and 'b'"); + } + + #[test] + fn push_parameter_list_three() { + let mut s = String::new(); + push_parameter_list(&mut s, &["a", "b", "c"]); + assert_eq!(&s, "'a', 'b', and 'c'"); + } + + #[test] + fn push_parameter_list_four() { + let mut s = String::new(); + push_parameter_list(&mut s, &["a", "b", "c", "d"]); + assert_eq!(&s, "'a', 'b', 'c', and 'd'"); + } +}