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

fix(types): remove newlines from docstring signature #4735

Merged
merged 30 commits into from
Aug 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c89e5b2
Remove newlines from docstring signature
JeanElsner Jul 8, 2023
214b200
Jean/dev (#1)
JeanElsner Jul 8, 2023
99cd8d0
style: pre-commit fixes
pre-commit-ci[bot] Jul 8, 2023
f795b7e
Don't use std::find_if for C++ 11 compatibility
JeanElsner Jul 8, 2023
9640613
Avoid implicit char to bool conversion
JeanElsner Jul 8, 2023
293e6e2
Merge branch 'pybind:master' into jean/patch/argval
JeanElsner Jul 21, 2023
d8e7d2d
Test default arguments for line breaks
JeanElsner Jul 24, 2023
418bdef
style: pre-commit fixes
pre-commit-ci[bot] Jul 24, 2023
4521195
Separate Eigen tests
JeanElsner Jul 24, 2023
d47aa39
style: pre-commit fixes
pre-commit-ci[bot] Jul 24, 2023
7a9382a
Fix merge
JeanElsner Jul 24, 2023
15196d1
Merge branch 'pybind:master' into jean/patch/argval
JeanElsner Jul 24, 2023
5d7e555
Try importing numpy
JeanElsner Jul 29, 2023
a2d675f
Avoid unreferenced variable in catch block
JeanElsner Jul 29, 2023
035a044
style: pre-commit fixes
pre-commit-ci[bot] Jul 29, 2023
19a8d51
Update squash function
JeanElsner Aug 7, 2023
122e15a
Reduce try block
JeanElsner Aug 7, 2023
e6be9d5
Additional test cases
JeanElsner Aug 7, 2023
1b5a3d7
style: pre-commit fixes
pre-commit-ci[bot] Aug 7, 2023
4c844d5
Merge branch 'pybind:master' into jean/patch/argval
JeanElsner Aug 7, 2023
9918f22
Put statement inside braces
JeanElsner Aug 7, 2023
25c9657
Move string into function body
JeanElsner Aug 7, 2023
0ea0a13
Rename repr for better readability. Make constr explicit.
JeanElsner Aug 7, 2023
a2add0d
Add multiline string default argument test case
JeanElsner Aug 7, 2023
1a9dd7b
style: pre-commit fixes
pre-commit-ci[bot] Aug 7, 2023
d53892c
Add std namespace, do not modify string repr
JeanElsner Aug 8, 2023
9cefda8
Test for all space chars, test str repr not modified
JeanElsner Aug 8, 2023
68c5643
Merge branch 'pybind:master' into jean/patch/argval
JeanElsner Aug 8, 2023
d3885da
Merge branch 'pybind:master' into jean/patch/argval
JeanElsner Aug 14, 2023
b391386
Merge branch 'pybind:master' into jean/patch/argval
JeanElsner Aug 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,45 @@ PYBIND11_WARNING_DISABLE_MSVC(4127)

PYBIND11_NAMESPACE_BEGIN(detail)

inline std::string replace_newlines_and_squash(const char *text) {
const char *whitespaces = " \t\n\r\f\v";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please sprinkle \r, \f, \v into the tests below?

What we want is that a test fails if any of the characters here is accidentally lost. (Imagine a silly typo slipping in while refactoring.)

std::string result(text);
bool previous_is_whitespace = false;

// Do not modify string representations
char first_char = result[0];
char last_char = result[result.size() - 1];
if (first_char == last_char && first_char == '\'') {
return result;
}
result.clear();

// Replace characters in whitespaces array with spaces and squash consecutive spaces
while (*text != '\0') {
if (std::strchr(whitespaces, *text)) {
if (!previous_is_whitespace) {
result += ' ';
previous_is_whitespace = true;
}
} else {
result += *text;
previous_is_whitespace = false;
}
++text;
}

// Strip leading and trailing whitespaces
const size_t str_begin = result.find_first_not_of(whitespaces);
if (str_begin == std::string::npos) {
return "";
}

const size_t str_end = result.find_last_not_of(whitespaces);
const size_t str_range = str_end - str_begin + 1;

return result.substr(str_begin, str_range);
}

// Apply all the extensions translators from a list
// Return true if one of the translators completed without raising an exception
// itself. Return of false indicates that if there are other translators
Expand Down Expand Up @@ -424,7 +463,7 @@ class cpp_function : public function {
// Write default value if available.
if (!is_starred && arg_index < rec->args.size() && rec->args[arg_index].descr) {
signature += " = ";
signature += rec->args[arg_index].descr;
signature += detail::replace_newlines_and_squash(rec->args[arg_index].descr);
}
// Separator for positional-only arguments (placed after the
// argument, rather than before like *
Expand Down
17 changes: 17 additions & 0 deletions tests/test_eigen_matrix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,23 @@ TEST_SUBMODULE(eigen_matrix, m) {
m.def("dense_c", [mat]() -> DenseMatrixC { return DenseMatrixC(mat); });
m.def("dense_copy_r", [](const DenseMatrixR &m) -> DenseMatrixR { return m; });
m.def("dense_copy_c", [](const DenseMatrixC &m) -> DenseMatrixC { return m; });
// test_defaults
bool have_numpy = true;
try {
py::module_::import("numpy");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome that this works on all platforms.

But this is a very large try block that could mask unrelated errors and create big surprises, in particular when refactoring bits of pybind11.

Could you please try if it still works like this:

bool have_numpy = true;
try {
    py::module_::import("numpy");
} catch (const py::error_already_set &) {
    have_numpy = false;
}

} catch (const py::error_already_set &) {
have_numpy = false;
}
if (have_numpy) {
py::module_::import("numpy");
Eigen::Matrix<double, 3, 3> defaultMatrix = Eigen::Matrix3d::Identity();
m.def(
"defaults_mat", [](const Eigen::Matrix3d &) {}, py::arg("mat") = defaultMatrix);

Eigen::VectorXd defaultVector = Eigen::VectorXd::Ones(32);
m.def(
"defaults_vec", [](const Eigen::VectorXd &) {}, py::arg("vec") = defaultMatrix);
}
// test_sparse, test_sparse_signature
m.def("sparse_r", [mat]() -> SparseMatrixR {
// NOLINTNEXTLINE(clang-analyzer-core.uninitialized.UndefReturn)
Expand Down
5 changes: 5 additions & 0 deletions tests/test_eigen_matrix.py
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,11 @@ def test_dense_signature(doc):
)


def test_defaults(doc):
assert "\n" not in str(doc(m.defaults_mat))
assert "\n" not in str(doc(m.defaults_vec))


def test_named_arguments():
a = np.array([[1.0, 2], [3, 4], [5, 6]])
b = np.ones((2, 1))
Expand Down
44 changes: 44 additions & 0 deletions tests/test_kwargs_and_defaults.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,50 @@ TEST_SUBMODULE(kwargs_and_defaults, m) {
m.def("kw_func_udl", kw_func, "x"_a, "y"_a = 300);
m.def("kw_func_udl_z", kw_func, "x"_a, "y"_a = 0);

// test line breaks in default argument representation
struct CustomRepr {
std::string repr_string;

explicit CustomRepr(const std::string &repr) : repr_string(repr) {}

std::string __repr__() const { return repr_string; }
};

py::class_<CustomRepr>(m, "CustomRepr")
.def(py::init<const std::string &>())
.def("__repr__", &CustomRepr::__repr__);

m.def(
"kw_lb_func0",
[](const CustomRepr &) {},
py::arg("custom") = CustomRepr(" array([[A, B], [C, D]]) "));
m.def(
"kw_lb_func1",
[](const CustomRepr &) {},
py::arg("custom") = CustomRepr(" array([[A, B],\n[C, D]]) "));
m.def(
"kw_lb_func2",
[](const CustomRepr &) {},
py::arg("custom") = CustomRepr("\v\n array([[A, B], [C, D]])"));
m.def(
"kw_lb_func3",
[](const CustomRepr &) {},
py::arg("custom") = CustomRepr("array([[A, B], [C, D]]) \f\n"));
m.def(
"kw_lb_func4",
[](const CustomRepr &) {},
py::arg("custom") = CustomRepr("array([[A, B],\n\f\n[C, D]])"));
m.def(
"kw_lb_func5",
[](const CustomRepr &) {},
py::arg("custom") = CustomRepr("array([[A, B],\r [C, D]])"));
m.def(
"kw_lb_func6", [](const CustomRepr &) {}, py::arg("custom") = CustomRepr(" \v\t "));
m.def(
"kw_lb_func7",
[](const std::string &) {},
py::arg("str_arg") = "First line.\n Second line.");

// test_args_and_kwargs
m.def("args_function", [](py::args args) -> py::tuple {
PYBIND11_WARNING_PUSH
Expand Down
32 changes: 32 additions & 0 deletions tests/test_kwargs_and_defaults.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,38 @@ def test_function_signatures(doc):
doc(m.KWClass.foo1)
== "foo1(self: m.kwargs_and_defaults.KWClass, x: int, y: float) -> None"
)
assert (
doc(m.kw_lb_func0)
== "kw_lb_func0(custom: m.kwargs_and_defaults.CustomRepr = array([[A, B], [C, D]])) -> None"
)
assert (
doc(m.kw_lb_func1)
== "kw_lb_func1(custom: m.kwargs_and_defaults.CustomRepr = array([[A, B], [C, D]])) -> None"
)
assert (
doc(m.kw_lb_func2)
== "kw_lb_func2(custom: m.kwargs_and_defaults.CustomRepr = array([[A, B], [C, D]])) -> None"
)
assert (
doc(m.kw_lb_func3)
== "kw_lb_func3(custom: m.kwargs_and_defaults.CustomRepr = array([[A, B], [C, D]])) -> None"
)
assert (
doc(m.kw_lb_func4)
== "kw_lb_func4(custom: m.kwargs_and_defaults.CustomRepr = array([[A, B], [C, D]])) -> None"
)
assert (
doc(m.kw_lb_func5)
== "kw_lb_func5(custom: m.kwargs_and_defaults.CustomRepr = array([[A, B], [C, D]])) -> None"
)
assert (
doc(m.kw_lb_func6)
== "kw_lb_func6(custom: m.kwargs_and_defaults.CustomRepr = ) -> None"
)
assert (
doc(m.kw_lb_func7)
== "kw_lb_func7(str_arg: str = 'First line.\\n Second line.') -> None"
)


def test_named_arguments():
Expand Down
Loading