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.

Sign up for GitHub

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

Jump to bottom

Remove abuse of arg_v. #1954

Merged
merged 2 commits into from
Aug 23, 2022
Merged

Remove abuse of arg_v. #1954

merged 2 commits into from
Aug 23, 2022

Conversation

thorstenhater
Copy link
Contributor

@thorstenhater thorstenhater commented Aug 19, 2022 •
edited
Loading

Do not abuse pybind::arg_v("name", T default, "description") as pybind::arg_v("name", "description").
Use descriptive names and pybind::arg instead.

Closes #1754

Helveg
Helveg reviewed Aug 20, 2022
View reviewed changes
Copy link
Collaborator

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

So, were all the argument descriptions dropped?

python/cells.cpp
Comment on lines +777 to +778
pybind11::arg("policy"),
"A cv_policy used to discretise the cell into compartments for simulation")
Copy link
Collaborator

Choose a reason for hiding this comment

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

These look like a different pattern, is this OK?

@thorstenhater
Copy link
Contributor Author

arg_v is for use with default values, which these do not have. Consequently I shuffled the parameter description
into the function docstring where applicatble.

brenthuisman
brenthuisman reviewed Aug 22, 2022
View reviewed changes
Copy link
Contributor

@brenthuisman brenthuisman left a comment

Choose a reason for hiding this comment

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

Most of them just removed the docstring, right? Not that they were terribly useful.

@thorstenhater
Copy link
Contributor Author

thorstenhater commented Aug 22, 2022 •
edited
Loading

Yes, where redundant I removed them.

@brenthuisman brenthuisman self-requested a review August 23, 2022 12:10
@brenthuisman brenthuisman merged commit b24245a into arbor-sim:master Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brenthuisman brenthuisman brenthuisman approved these changes

@Helveg Helveg Helveg approved these changes

@AdhocMan AdhocMan Awaiting requested review from AdhocMan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pybind seems to interpret some decor.paint docstrs as default values
3 participants
@thorstenhater @brenthuisman @Helveg