Skip to content

Added write only property functions for issue #1142 #1144

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

Merged
merged 3 commits into from
Nov 7, 2017

Conversation

TD22057
Copy link
Contributor

@TD22057 TD22057 commented Oct 12, 2017

Added support for write only properties using the def methods:

  • def_writeonly
  • def_writeonly_static
  • def_property_writeonly
  • def_property_writeonly_static

The lowest level property implementation in def_property_static() was updated to check to see if the fget pointer is null to allow for these cases. Tests cases added and run. Python handles the non-readable attribute the same way it does in regular Python so the error is identical to that in #1142.

>>> msr.ignore = "asdf"
>>> msr.ignore
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: unreadable attribute

@jagerman
Copy link
Member

This PR is adding quite a few methods to class_ for what is essentially a pretty esoteric feature. I'm not saying we shouldn't support it, but I think we could reduce the footprint somewhat by allowing fget to be passed py::none. That could probably be done easily enough by adding a do-nothing cpp_function(const py::none &) {} constructor. Then you could use something like:

cl.def_property("ignore", py::none, [](Cls &self, const int &val) { self.ignore = val; });

to declare the property. It's not quite as elegant, but on the other hand avoids adding 6 templated methods to class_.

@jagerman
Copy link
Member

It also has the advantage of making the API closer to the Python API for doing the same thing.

@wjakob
Copy link
Member

wjakob commented Oct 17, 2017

I agree with @jagerman -- the fact that nobody needed this until now indicates how rare of a use case it is.

@TD22057
Copy link
Contributor Author

TD22057 commented Oct 17, 2017

No problem, I was just trying to follow pattern what there for read only properties. Can you suggest how to do that? Right now, those are c++ functions that accept either a template pointer (D* pm), const Getter&, or const cpp_function&. How does one convert py::none to those?

This might be easier to implement it this way:
cl.def_property("ignore", nullptr, [](Cls &self, const int &val) { self.ignore = val; });

@jagerman
Copy link
Member

If there's a cpp_function(const none &) { } constructor you should be able to pass py::none() as a cpp_function argument via implicit conversion. (I accidentally left off the () in my previous message).

@TD22057
Copy link
Contributor Author

TD22057 commented Oct 20, 2017

So I tried to make this work today and ran into issues. For the functions def_writeonly() and def_writeonly_static(), they can't be implemented using def_readwrite() and def_readwrite_static() because those definitions look like this:

    template <typename C, typename D, typename... Extra>
    class_ &def_readwrite(const char *name, D C::*pm, const Extra&... extra) {
        static_assert(std::is_base_of<C, type>::value, "def_readwrite() requires a class member (or base class member)");
        cpp_function fget([pm](const type &c) -> const D &{ return c.*pm; }, is_method(*this)),
                     fset([pm](type &c, const D &value) { c.*pm = value; }, is_method(*this));
        def_property(name, fget, fset, return_value_policy::reference_internal, extra...);
        return *this;
    }

They basically don't pass in 2 function pointers - just the class and attribute name. So a writeonly template method has to be implemented for those.

For properties (def_property_writeonly() and def_property_writeonly_static()), those can be implemented by passing py::cpp_function() as the fget argument. py::none and py::none() won't compile - it works when using a function pointer, but not when using a lambda. py::cpp_function() works in both cases.

So which would you prefer? My opinion is that since writeonly is required for attributes, it makes sense to add it for properties for consistency. AFAIK, there is no code bloat issue since these shouldn't be instantiated if they aren't used. Otherwise there are two different calls to do writeonly but readwrite and readonly are consistent. It's your library though so let me know which you'd prefer.

@jagerman
Copy link
Member

For the functions def_writeonly() and def_writeonly_static(), they can't be implemented using def_readwrite() and def_readwrite_static()

Sure, but they can be implemented using def_property, which is the generic version taking an arbitrary getter/setter (and is already called by def_readwrite now). It means for a write-only attribute, you have to provide a lambda for the setter: def_property("foo", py::none() /* or nullptr or whatever */, [](MyType &self, const double foo) { self.foo = foo; })--so slightly more verbose, but as I said before, a bit more verbosity required for rare cases seems fine to me.

@TD22057
Copy link
Contributor Author

TD22057 commented Oct 23, 2017

OK. Are you ok if the syntax uses py::cpp_function() as the "none" input?

I tried to implement it to allow nullptr or py::none() and it's quite a bit more code (and I quit before I got it to actually work) because def_property wraps the input in a method_adapter which does something weird with nullptr before it passes it to cpp_function. Using cpp_function() means the only code change would be the updates to def_property_static() and def_property_static_impl() to allow for the getter to be unset.

I updated the test case to use this syntax and it makes the various write only tests look like this:

.def_property("def_writeonly", py::cpp_function(),
              [](TestProperties& s,int v) { s.value = v; } )
.def_property("def_property_writeonly", py::cpp_function(), &TestProperties::set)
.def_property_static("def_writeonly_static", py::cpp_function(),
                     [](py::object, int v){ TestProperties::static_value = v; })
.def_property_static("def_property_writeonly_static", py::cpp_function(),
                     [](py::object, int v) { return TestProperties::static_set(v); })

If you think that's ok, I'll update the docs w/ an example for a write only property and that should be it.

@jagerman
Copy link
Member

I tried to implement it to allow nullptr or py::none() ...

You should be able to add these two constructors to cpp_function to make both approaches work:

    cpp_function(std::nullptr_t) { }
    cpp_function(none) { }

It means you have to pass a literal nullptr, not just a null pointer of some other type, but it appears to work (though I only tested under gcc; I suppose it's possible that some other compilers in the CI suite have trouble with it).

That said, we probably don't need both here; I think sticking with just the nullptr_t argument support (i.e. not adding the none constructor) is enough.

@TD22057
Copy link
Contributor Author

TD22057 commented Oct 25, 2017

@jagerman I make the update use to nullptr_t which worked fine (thanks for that). When I tried to push it back, git complained about the branch being out of date. I thought I was "fixing" that but it seems to have updated way too many files with changes that I didn't actually make. I'm a git novice so I have no idea what to do at this point (besides delete the branch and start over with my changes). Any help would be appreciated (sorry).

@jagerman
Copy link
Member

Usually I use git rebase -i upstream/master (where upstream is your local name for the main GitHub remote). That'll also let you merge and squash commits (if you want), and will reset your commits to the top of the current master branch. As it goes, it'll stop when there is a conflict and give you instructions on resolving it and continuing.

py::class_<T>'s `def_property` and `def_property_static` can now take a
`nullptr` as the getter to allow a write-only property to be established
(mirroring Python's `property()` built-in when `None` is given for the
getter).
@jagerman
Copy link
Member

I force-pushed a change to your branch that squashes everything into one commit, and rebases it on current master. You're going to get conflicts if you try to pull it now, but you can do a hard reset to the github version with:

git fetch origin
git reset --hard origin/writeonly_property

If you've made local changes since what was added to the branch, you'll want to git stash them first; you can try git stash pop to reapply them after resetting the branch, but if that fails, git stash show -u will show the changes which you can then apply back on the reset branch as new commits.

I have a couple minor suggestions that I'll leave as comments, but overall I think this looks pretty good.

@@ -51,6 +51,7 @@ NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
class cpp_function : public function {
public:
cpp_function() { }
cpp_function(std::nullptr_t) { }
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty minor, but I think with this nullptr constructor it would be slightly nicer to change def_property_readonly and def_property_readonly_static to pass nullptr rather than cpp_function(). It won't make any difference to how it works, but conceptually it seems nicer since the PR adds (and documents) passing a nullptr.

Copy link
Member

Choose a reason for hiding this comment

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

Related to this: it should also be possible to specify a default-constructed cpp_function() for either the setter or getter, so perhaps add another property such as:

.def_property("impossible", py::cpp_function(), py::cpp_function())

with associated tests for attribute errors on both set or get (as per my other comment), but mainly just so we still have some test suite code invoking that code path.

if (rec_fget->doc && rec_fget->doc != doc_prev) {
free(doc_prev);
rec_fget->doc = strdup(rec_fget->doc);
}
Copy link
Member

Choose a reason for hiding this comment

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

One consequence of this is that you can now do:

cl.def_property("x", nullptr, nullptr)

and get an x that can be neither read nor assigned to. I have no idea why someone would ever want to do this, but Python's property() allows it, so I suppose we can just let it slide as well:

>>> class A:
...     x = property(None, None)
... 
>>> a = A()
>>> a.x
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: unreadable attribute
>>> a.x = 2
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: can't set attribute

(So in other words, no change needed here, just an observation in passing).

@@ -98,6 +98,11 @@ def test_properties():
instance.def_property = 3
assert instance.def_property == 3

with pytest.raises(AttributeError):
dummy = instance.def_property_writeonly # noqa: F841 unused var
Copy link
Member

Choose a reason for hiding this comment

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

This should have a assert "unreadable attribute" in str(excinfo) after it (the later ones all have it, just this one looks to be missing the assertion value check).

@jagerman
Copy link
Member

One last comment - add a changelog entry to docs/changelog.rst under v2.3.0.

@TD22057
Copy link
Contributor Author

TD22057 commented Oct 30, 2017

Thanks for the help. Since I thought I knew what I was doing, I didn't keep track of what I did so it was hard to undo whatever weird state I had gotten the branch into. I think I've incorporated all of your requests.

Copy link
Member

@jagerman jagerman left a comment

Choose a reason for hiding this comment

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

This PR looks good to me.

I'll wait a couple days to see if anyone else has comments.

@wjakob
Copy link
Member

wjakob commented Nov 7, 2017

This looks great, feel free to merge @jagerman.

@jagerman jagerman merged commit 0a0758c into pybind:master Nov 7, 2017
@jagerman
Copy link
Member

jagerman commented Nov 7, 2017

Merged! Thanks for the PR, @TD22057 !

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