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

Add missing function parameter names of pybind11 generated functions #2222

Merged
merged 3 commits into from
Jun 29, 2022

Conversation

atouminet
Copy link
Contributor

Fixes #2220

@nate-sime
Copy link
Contributor

Is there a reason this isn't automatically available through pybind11? Maintaining these signatures could be cumbersome.

@atouminet
Copy link
Contributor Author

I guess it is just not possible. From the pybind11 perspective, the function to wrap is just an object wrapping a function pointer. In C++ these pointers do not store the names of the function arguments.

One thing that helps with maintainability though is that pybind11 will run some static asserts to check if the number of arguments matches the number of py::arg defined in the code, such that if some arguments are added or removed from the C++ functions, not updating the wrappers will result in a build error. However swapping or renaming arguments will require manual intervention.

The same could be said for the C++ API documentation that is not translated automatically to the python API.
I guess that's just how it is when working with multiple languages.

@nate-sime
Copy link
Contributor

I like the idea. Also for some reason your IDE or code linting has over formatted your code.

@atouminet
Copy link
Contributor Author

Are you sure about the formatting? I made sure to run clang-format on each file. Just checked again and not seeing any diffs compared to the current version.

@jhale
Copy link
Member

jhale commented Jun 1, 2022

https://github.com/FEniCS/dolfinx/runs/6676578989?check_suite_focus=true

The formatting is OK according to the CI.

Copy link
Contributor

@nate-sime nate-sime left a comment

Choose a reason for hiding this comment

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

This is definitely helpful for users, I just worry about maintenance.

@garth-wells
Copy link
Member

@atouminet could you resolve the merge conflict?

@garth-wells garth-wells added the enhancement New feature or request label Jun 24, 2022
@atouminet
Copy link
Contributor Author

@garth-wells just rebased the PR and resolved the conflicts.

@garth-wells garth-wells merged commit eef67ee into FEniCS:main Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Function argument names are wrong in the pybind11 generated functions
4 participants