Skip to content

Use default arguments in create_extension_module #2717

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

Conversation

martinRenou
Copy link
Contributor

@martinRenou martinRenou commented Dec 8, 2020

Description

We used to be able to create a module with a simple

py::module my_module("module");

having to write the following to create a module is more verbose, less clean visually.

py::module my_module = py::module_::create_extension_module("module", nullptr, new py::module_::module_def);

This PR allows to write the following instead:

py::module my_module = py::module_::create_extension_module("module");

@martinRenou
Copy link
Contributor Author

Would you like to consider de-deprecating the previous API? It was way nicer than the new one.

@SylvainCorlay
Copy link
Member

I was opening #2718 at the same time.

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Dec 8, 2020

Thanks for the suggestion and clear description!

I just commented in #2718 (#2718 (comment)) the reasoning on the changes and current API, and why I don't think this is the correct way to go.

@YannickJadoul
Copy link
Collaborator

Let's continue the discussion in #2718, before there's too much confusion?

@YannickJadoul
Copy link
Collaborator

Given comments in #2718 and arguments about why this PR should not be merged, I'm going to close this? I thought I had conveinced @martinRenou and @SylvainCorlay in #2718 that #2551 is (probably) the way forward?

If not, apologies; and please reopen.

@martinRenou martinRenou deleted the create_extension_modules_default_args branch December 15, 2020 08:13
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