Skip to content

refactor: Replace h.get_type() with py::type(h) #2388

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

Closed
henryiii opened this issue Aug 12, 2020 · 10 comments · Fixed by #2492
Closed

refactor: Replace h.get_type() with py::type(h) #2388

henryiii opened this issue Aug 12, 2020 · 10 comments · Fixed by #2492
Assignees
Labels
policy Suggestions for changes in policy

Comments

@henryiii
Copy link
Collaborator

henryiii commented Aug 12, 2020

With the addition of #2364, a very natural extension to py::type<T>() would be py::type(h), mimicking py::cast. I would propose we depreciate and eventually remove h.get_type() in favor of py::type(h). Reasons:

  • Free functions are generally better than member functions in C++; they are simpler, composable, etc.
  • It would simplify the mixin, which currently gets added to several different classes, and keeps the API closer to Python.
  • h.str() is already deprecated in favor of py:str(h).

It is only used internally, not publicly in any docs or tests, so to me it seems it would read better (more python like) and simplify the mixin.

For example, randomly picking one of the 10 places it's currently used:

if (!obj.get_type().is(matrix_type))
// Becomes
if (!py::type(obj).is(matrix_type))

For comparison, the Python would be:

if not type(obj) is matrix_type:

(Discussion moved from #2364).

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Aug 12, 2020

👍 Yup, you already convinced me in #2364! ;-)

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Aug 14, 2020

We could make this similar to py::str, though, and solve #1692 at the same time. (#2364 hasn't been released yet, so we can still change that interface?)

@henryiii
Copy link
Collaborator Author

Technically, I think that issue would be solved by py::type<std::string>()(), if we supported str/int/etc. (which we don't yet, in that PR)? I don't think there was a need for a typeobject type expressed there, but rather a way to get the type as a handle?

@YannickJadoul
Copy link
Collaborator

Right, but I'm thinking we could create a class type : public object {, just like with str? That would allow to e.g. use it in signatures, call py::insinstance<py::type>(...), etc.

@henryiii
Copy link
Collaborator Author

Then we would lose py::type<T>(), which only works if py::type is a function. I'm not sure we would really gain much from it being a class. Also, you'd run in to the same issue with str, where it can't be a handle since it's based on object.

@YannickJadoul
Copy link
Collaborator

Agh, I had naively thought/hoped you could make make a templated template <typename T> type() {} constructor, but apparently that's not allowed for constructors :-(

Best I can then think of is:

class type {
public:
    template <typename T>
    static type of() { return type(typeid(T)); }

    explicit type(const std::type_info &type_info) : m_type_info(type_info) {}

    void print() {
        std::cout << m_type_info.name() << std::endl;
    }

private:
    const std::type_info& m_type_info;
};

This of<T> pattern already is present for dtype, I believe.

Not sure if that's worth it, but I do like the conformity of having a py::type class for PyType_Type (just like we have py::int_, py::dict, etc).
Then again, I'm not sure if this is now sooo crucial. I don't see if people would use it that much, so I can live with "just" making this a function. But worth mentioning/discussing, at least, before we fix this API and can never use py::type again for anything else.

@henryiii
Copy link
Collaborator Author

I'd be okay with py::type::of<T>(); not quite as nice, but py::type for PyType_Type sounds good.

@rwgk
Copy link
Collaborator

rwgk commented Aug 17, 2020

FWIW: I looked in the Google codebase: I see .get_type() used, but only in 8 files, 14 hits total (we have ~360 pybind11 extensions). — This looks like a fairly easy change, if there is a deprecation period.

@YannickJadoul
Copy link
Collaborator

I'd be okay with py::type::of<T>(); not quite as nice, but py::type for PyType_Type sounds good.

I know :-/ I'm quite conflicted as well. I quite like how py::type<T>() looks, and it makes sense (similar to py::cast, etc), but ...
Of course, there's probably nothing more py::type (wrapping PyType_Type) would do, compared to py::object, apart from py::isinstance?

@wjakob, any thoughts on this? Having py::type<T>() and py::type(handle) as functions (conform py::cast), vs. having py::type as a class wrapping PyType_Type with a py::type::of<T>() and a py::type(handle) converting constructor (conform e.g. py::str and py::dtype) ?

@henryiii henryiii added the policy Suggestions for changes in policy label Aug 26, 2020
@wjakob
Copy link
Member

wjakob commented Sep 2, 2020

+1 for this interface in general. I'm kind of torn about the specifics of naming it as well, but leaning slightly towards consistency with py::str, which also has the advantage of subsuming everything under a common prefix. (Otherwise we'd have find some different identifier for PyType_Type, which seems like it's going to become very confusing).

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

Successfully merging a pull request may close this issue.

4 participants