Skip to content

[BUG]: the new native_enum does not support class doc strings #5615

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
3 tasks done
dyollb opened this issue Apr 14, 2025 · 3 comments · Fixed by #5617
Closed
3 tasks done

[BUG]: the new native_enum does not support class doc strings #5615

dyollb opened this issue Apr 14, 2025 · 3 comments · Fixed by #5617
Labels
triage New bug, unverified

Comments

@dyollb
Copy link
Contributor

dyollb commented Apr 14, 2025

Required prerequisites

What version (or hash if on master) of pybind11 are you using?

master

Problem description

py::enum_ supported class docstrings (as did boost::python::enum_). It would be nice if the new py::native_enum would support class docstrings.

Since the native enum type (a string) is optional, converting my code to use thew new native_enum forced me to review each instance, since by default my docstrings were interpreted as the native enum. Maybe the native enum could be an enum or a class type, so we could have a constructor like

template <typename... Extra>
native_enum(const object &parent_scope,
                const char *name,
                const Extra &...extra)

where the extra arguments could be a docstring and/or a native enum.

This would allow a "complete" replacement and less error-prone conversion to the new native_enum. An alternative could be to add a method native_enum::set_doc(const char* doc).

Reproducible example code


Is this a regression? Put the last known working version here if it is.

Not a regression

@dyollb
Copy link
Contributor Author

dyollb commented Apr 14, 2025

Something like this could work (but break backwards compatibility with code written in the last 2-3 weeks):

struct native_base {
    std::string name;
    explicit native_base(std::string name) : name(std::move(name)) {}
};

template <typename... Args>
const char* extract_docstring(const Args&... args) {
    const char* doc = "";
    (void(std::initializer_list<int>{
        (std::is_convertible<Args, const char*>::value ? (doc = args, 0) : 0)...}));
    return doc;
}

template <typename... Args>
std::string extract_baseclass(const Args&... args) {
    std::string base = "enum.Enum";

    // Fold over all args
    (void)std::initializer_list<int>{
        ([&]{ // inline lambda for each arg
            if constexpr (std::is_same<typename std::decay<Args>::type, native_base>::value) {
                base = args.name;
            }
            return 0;
        }(), 0)...};

    return base;
}

/// Conversions between Python's native (stdlib) enum types and C++ enums.
template <typename EnumType>
class native_enum : public detail::native_enum_data {
public:
    using Underlying = typename std::underlying_type<EnumType>::type;

    template <typename... Extra>
    native_enum(const object &parent_scope,
                const char *name,
                const Extra &... extra)
        : detail::native_enum_data(
              parent_scope, name, extract_baseclass(extra...).c_str(), extract_docstring(extra...), std::type_index(typeid(EnumType))) {

Usage example

py::native_enum<eDialog>(m, "eDialog", py::native_base("enum.IntEnum"), "Constants for dialog response")
    .value("Ok", eDialog::Ok)
    .value("Fail", eDialog::Fail)
    .finalize();

py::native_enum<eMessageType>(m, "eMessageType", "Constants for message type")
    .value("Text", eMessageType::Text)
    .value("Image", eMessageType::Image)
    .finalize();

Questions

  • would you want to wrap the native enum type (see above: NativeEnum) or the docstring? Wrapping the native type would more easily allow switching from enum_ to native_enum
  • maybe pybind11 already has helper functions to unpack the parameter pack
  • what minimum C++ standard is required (and available) for pybind11? Can I use C++17?

@rwgk
Copy link
Collaborator

rwgk commented Apr 14, 2025

but break backwards compatibility with code written in the last 2-3 weeks

I think that'd be OK.

Maybe simpler is better here?

Concretely, we could make native_type_name mandatory again: "explicit is better than implicit" would win here

Then the additional docstring could be optional.

@rwgk
Copy link
Collaborator

rwgk commented Apr 14, 2025

I forgot to add: I'll review quickly if you open a PR.

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

Successfully merging a pull request may close this issue.

2 participants