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

DM-31535: Deprecate default-position-argument interface to Psf methods. #605

Merged
merged 2 commits into from Sep 9, 2021

Conversation

jmeyers314
Copy link
Contributor

No description provided.

# You should have received a copy of the GNU General Public License
# along with this program. If not, see <https://www.gnu.org/licenses/>.

__all__ = ["Psf"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure you don't need or want this. This file is only used for its side effects.

setattr(
Psf,
method,
deprecate_pybind11(
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is going to show up too frequently, I think it is possible to wrap these differently and explicitly issue a warning only on the bad overload. It's a bit complex, though, which is why the Dev Guide doesn't mention it, and it may not be worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you suggest this work? My thought was to use a lambda when binding to manually emit a warning before passing through to the wrapped method. Maybe there's a way to do this just within python though?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an example of how this was done in the past using a Python-only patch: 488744f#diff-3428060cb78fe305ebd4f720718710a2421cc76d4b355393edb26ab6a74ce0d0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I considered inspecting the args like this but decided against it since it doesn't accomplish the sphinx decoration that deprecate_pybind11 does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if we're okay with ignoring the sphinx docs mangling, I think I'd prefer doing the same thing without the argument inspection; i.e., in _psf.cc something like:

                // cls.def("computeImage", py::overload_cast<>(&Psf::computeImage, py::const_));
                cls.def("computeImage",
                        [](const Psf& psf) {
                            auto warnings = py::module::import("warnings");
                            auto builtins = py::handle(PyEval_GetBuiltins());
                            auto FutureWarning = builtins["FutureWarning"];
                            warnings.attr("warn")(
                                "This is the warning",
                                "category"_a=FutureWarning,
                                "stacklevel"_a=2
                            );
                            return psf.computeImage();
                        }
                );

I assume there's a way to make this into a template too, though that might be a bit beyond my skillset.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any of the three options is OK with me: deprecate_pybind11, warn in Python, warn in the wrapper.

@jmeyers314 jmeyers314 merged commit 197addc into master Sep 9, 2021
@jmeyers314 jmeyers314 deleted the tickets/DM-31535 branch September 9, 2021 17:06
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.

None yet

2 participants