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

Move cpp_function to its own header file for further refactoring #2807

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lqf96
Copy link
Contributor

@lqf96 lqf96 commented Jan 19, 2021

Description

Move cpp_function to its own header file pybind11/detail/function.h, because I find it hard to refactor cpp_function as pybind11/pybind11.h is too long. I specifically choose the detail directory as I'd like to split cpp_function into two parts in the long term: one part (in pybind11 namespace) being an object wrapper around a C++ function and another part (in detail namespace) dispatching the actual function call. I'll provide more details about these refactoring in upcoming PRs. This PR also moves signature and docstring generation of cpp_function to separate functions.

if (!some_args) some_args = true;
else msg += ", ";
try {
msg += repr(args_[ti]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason this was changed from pybind11::repr to repr? It should be perfectly fine, since args_[ti] has a type from the pybind11 namespace, so even ADL would only find pybind11::repr, just curious if the change was intentional.

@YannickJadoul
Copy link
Collaborator

I'd like to first get some context on this refactoring, before moving things around. Can you please create an issue with what you're planning to do and, more importantly, why, so that we can discuss? Anything that's refactoring something this big should be discussed with maintainers, before just throwing a series of PRs at us, since it has implications for existing PRs and forks, and shouldn't just be done "for fun".

Take the interaction with #2445 (first proposed/discussed in #2322) as an example of why this needs to be coordinated.

@lqf96
Copy link
Contributor Author

lqf96 commented Jan 19, 2021

@YannickJadoul Yeah I agree we should probably discuss about this in advance... Let me open an issue later about the motivations and intentions...

@lqf96
Copy link
Contributor Author

lqf96 commented Jan 19, 2021

@bstaletic @YannickJadoul I just opened #2810 to describe the problem I ran into, the refactors I want to do and the potential problems. It is more like a RFC so feel free to give me suggestions there.

@YannickJadoul
Copy link
Collaborator

@lqf96, thanks, I saw! When I (soon) pull together my courage to think about this, I'll have a closer look and comment in that PR :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants