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

check for already existing enum value added; added test #1453

Conversation

Projects
None yet
4 participants
@cysiek
Copy link
Contributor

commented Jul 12, 2018

Fix for #1451 (Defining enum with mutiple identical values should cause an error)
Now trying to add already existing value throws an exception.

@bstaletic

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

That error on Travis is definitely unrelated to your changes.

As for your PR, it looks good. I personally like pyname more than name_converted, but that's a matter of taste.

@cysiek

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2018

So this PR will be accepted after travis fix?

About the travis issue - looking at last successful build and the next one, the difference seems to be in python version:
https://travis-ci.org/pybind/pybind11/jobs/398351097#L1460 - successful
https://travis-ci.org/pybind/pybind11/jobs/398837022#L1461 - broken
Than at
https://travis-ci.org/pybind/pybind11/jobs/398837022#L1544 travis is trying to use python 3.6 and fails because the installed python3 version is 3.7.0. @bstaletic could you fix it?

@bstaletic

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

I can't say anything about this getting merged because I don't have write access to the repo.
The travis fix is in #1454.

m_entries[pybind11::str(name)] = std::make_pair(v, doc);
auto name_converted = pybind11::str(name);
if (m_entries.contains(name_converted))
throw value_error("Enum error - element with provided name already exist");

This comment has been minimized.

Copy link
@jagerman

jagerman Jul 17, 2018

Member

Let's put the name into this to make it easier to figure out which enum is the problem:

    throw value_error("Enum error - element " + std::string(name) + " already exists");

This comment has been minimized.

Copy link
@cysiek

cysiek Jul 17, 2018

Author Contributor

Good idea! The change is already on branch, so I think that now after travis fix, this can be merged to master.

@jagerman

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

The change looks good to me, but the test case needs to be moved (this really isn't related to the embedded interpreter). My apologies; I should have mentioned this earlier, but didn't notice until just now.

For tests like this that raise an exception during binding you need to defer the registration via a callable function, like this (in test_enum.cpp):

    // test_duplicate_enum_name
    m.def("register_bad_enum", [m]() {
        py::enum_<SimpleEnum>(m, "SimpleEnum")
            .value("ONE", SimpleEnum::ONE)          //NOTE: all value function calls are called with the same first parameter value
            .value("ONE", SimpleEnum::TWO)
            .value("ONE", SimpleEnum::THREE)
            .export_values();
    });

and then call/catch/check the error message from test_enum.py with:

def test_duplicate_enum_name():
    with pytest.raises(ValueError) as excinfo:
        m.register_bad_enum()
    assert str(excinfo.value) == "Enum error - element with name: ONE already exists"
@cysiek

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2018

Ok, i've moved the test case.

Wow, I didn't even knew that it's possible to defer the registration. Nice trick, thanks!

@cysiek

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

So... now this pull request will be accepted? Or something is missing?

@wjakob wjakob merged commit 5c8746f into pybind:master Sep 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wjakob

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

Merged, thank you!

vanossj added a commit to vanossj/pybind11 that referenced this pull request Oct 2, 2018

check for already existing enum value added; added test (pybind#1453)
* check for already existing enum value added; added test

* added enum value name to exception message

* test for defining enum with multiple identical names moved to test_enum.cpp/py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.