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

add @classproperty #5901

Merged
merged 17 commits into from Jun 6, 2018

Conversation

Projects
None yet
5 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Jun 4, 2018

Problem

There's no way to declare a method to be accessible as a property of the class, instead of every instance.

Solution

  • add @classproperty and @staticproperty decorators to meta.py
  • add the memoized forms of the above to memo.py, as well as their method forms: memoized_classproperty, memoized_staticproperty, memoized_staticmethod, memoized_classmethod
  • add testing for all of the above
  • consume @memoized_classproperty in the definition of GlobMatchErrorBehavior

@stuhood stuhood requested review from benjyw , jsirois and kwlzn Jun 4, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 4, 2018

Thanks @cosmicexplorer!

Sidenote: recommend installing the commit hooks from https://www.pantsbuild.org/howto_contribute.html#getting-pants-source-code to catch lint errors. You can easily skip them with -n, ie: git commit -a -m 'Blah' -n. And if you've been editing rust code and don't want to do a --release compile in order to lint, the MODE flag continues to work in that context: MODE=debug git commit -a -m 'Blah'.

@benjyw

benjyw approved these changes Jun 4, 2018

Copy link
Contributor

benjyw left a comment

Nice!

@kwlzn

kwlzn approved these changes Jun 4, 2018

Copy link
Member

kwlzn left a comment

lgtm mod one concern about class vs instance scope testing.


class ClassPropertyTest(TestBase):
# TODO: The assertions on both the class and an instance of it might not be necessary, if class
# attributes are assumed to always be the same on their instances.

This comment has been minimized.

@kwlzn

kwlzn Jun 4, 2018

Member

I think it's entirely possible (and an existing pattern in pants' code base) that an instance could inadvertently override a class variable - so having a test for that case up front would make a lot of sense to me - if for nothing else to clarify the expected behavior here.

i.e. I would expect this test to pass:

[omerta pants2 (pr5901)]$ git diff | cat
diff --git a/tests/python/pants_test/util/test_meta.py b/tests/python/pants_test/util/test_meta.py
index 4de0da6..d91e12a 100644
--- a/tests/python/pants_test/util/test_meta.py
+++ b/tests/python/pants_test/util/test_meta.py
@@ -53,6 +53,11 @@ class OverridingValueField(WithProp):
   _value = 4
 
 
+class OverridingValueInit(WithProp):
+  def __init__(self, v):
+    self._value = v
+
+
 class OverridingMethodDefSuper(WithProp):
 
   _other_value = 2
@@ -77,6 +82,9 @@ class ClassPropertyTest(TestBase):
     self.assertEqual(4, OverridingValueField.some_property)
     self.assertEqual(4, OverridingValueField().some_property)
 
+  def test_override_inst_value(self):
+    self.assertEqual(4, OverridingValueInit(3).some_property)
+
   def test_override_method_super(self):
     self.assertEqual(5, OverridingMethodDefSuper.some_property)
     self.assertEqual(5, OverridingMethodDefSuper().some_property)

but noting that it does not currently appear to work as expected:

 ==================== FAILURES ====================
 ___ ClassPropertyTest.test_override_inst_value ___
 
 self = <pants_test.util.test_meta.ClassPropertyTest testMethod=test_override_inst_value>
 
     def test_override_inst_value(self):
 >     self.assertEqual(4, OverridingValueInit(3).some_property)
 E     AssertionError: 4 != 3
 
 .pants.d/pyprep/sources/2a69993898e39901cfba78c09ed3eaea1cc1406f/pants_test/util/test_meta.py:86: AssertionError
  generated xml file: /Users/kwilson/dev/pants2/.pants.d/test/pytest/tests.python.pants_test.util.meta/junitxml/TEST-tests.python.pants_test.util.meta.xml 
 
 
 ====== 1 failed, 11 passed in 0.86 seconds =======

either way, cementing this in with a test (whether xfail or not) and some clarifying docs/notes about the behavior in this situation would probably be good.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 4, 2018

Contributor

Hm. I think it could make sense for that test to pass, but I get the same behavior you describe when using an @classmethod. It feels like doing whatever @classmethod does would be the least surprising thing for @classproperty to do -- even if it's potentially surprising to silently switch to using the instance field. Does that seem reasonable?

I'm adding your test case, as well as a couple assertions for an @classmethod to confirm that the behavior is the same when accessed on an instance. I am also going to add an explanation of this behavior in the @classproperty doc string.

This comment has been minimized.

@jsirois

jsirois Jun 4, 2018

Member

I think the failure makes sense since the attribute is accessed through an instance and not through the class. The mental model of lookup proceeding from most specific to least (ie: self, then cls/static) is retained.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 4, 2018

Contributor

Let me know if 31e1a38 is the kind of documentation/testing you were thinking of.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 4, 2018

Contributor

accessed through an instance and not through the class

Phrasing it like this made it more clear to me. I made a hacky subclass of the pex Installer at one point which overrode an @classmethod with an instance method when first trying to make native compilation work with local dists -- this ended up getting deleted in favor of a different approach but even though shadowing could happen by accident, that is also a feature that can be used intentionally (I recognize that overriding the method is a little different than making a shadowed field).

This comment has been minimized.

@kwlzn

kwlzn Jun 4, 2018

Member

works for me!

(ultimately, I don't have a strong view either way - the concern here is just that we have a documented stance on that behavior vs someone with mismatched expectations stumbling upon a TODO)

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 4, 2018

Contributor

Yes! I really appreciate you pointing this out @kwlzn because I can see a future self getting highly flummoxed without taking the time to make it clear here. I've removed that TODO in 81d3d6c -- let me know if the @classproperty docstring added in 31e1a38 is sufficient documentation, or if there could be a note elsewhere.

@jsirois

jsirois approved these changes Jun 4, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 4, 2018

It feels odd to me that @classproperty and @memoized_method can be combined, given that we have a @memoized_property decorator... do both combinations work? If so, could we do something to discourage arbitrary mixing of those?

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 4, 2018

here's what you get if you try to merge the two:

 self = <pants_test.util.test_memo.MemoizeTest testMethod=test_memoized_class_methods>
                     
   def test_memoized_class_methods(self):
      class Foo(object):
        _x = 3
                         
       @classmethod
       @memoized_method
       def method(cls, y):
         return cls._x + y
                         
       @classproperty
       @memoized_property
       def prop(cls):
         return cls._x
                         
>     self.assertEqual(3, Foo.prop)
.pants.d/pyprep/sources/af16b372569ceca4731a44b84de29461251eceb3/pants_test/util/test_memo.py:256: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <pants.util.meta.ClassPropertyDescriptor object at 0x7f5e522a5990>
obj = None
objtype = <class 'pants_test.util.test_memo.Foo'>
                     
   def __get__(self, obj, objtype=None):
      if objtype is None:
        objtype = type(obj)
>     return self.fget.__get__(obj, objtype)()
E     TypeError: 'property' object is not callable

There may be a way to get around this, but the idea is that (in my understanding) we can't use the property() constructor like @memoized_property does because that binds methods to a class instance, not the class itself (which is what ClassPropertyDescriptor is for).

I had added memoized_classmethod and memoized_classproperty to memo.py previously to eliminate the ambiguity, but realized I might also want to add e.g. memoized_staticmethod and more if we're going to open up that can of worms.

Or rather, that was my thought process, but thinking about it now, I can see @memoized_classproperty resolving the ambiguity without further issue (and I don't see a can of worms -- either a staticmethod has > 0 args, in which case it can't be a @classproperty, or it has 0 args, in which case I believe @memoized_classproperty (which is basically just @classproperty\n@memoized_method) would work. I will try that now.

EDIT: ok, staticmethod doesn't work that easily, and will require another decorator. While @classmethod\n@memoized_method seems fine to me, it seems reasonable to define @memoized_classmethod as its own thing as well.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 4, 2018

Added a commit which adds more memoized_* decorators to remove ambiguity, will clean up the tests in a sec.

cosmicexplorer added some commits Jun 4, 2018

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:class-properties-with-memoization branch from a374b5b to ab8e397 Jun 4, 2018

cosmicexplorer added some commits Jun 4, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 5, 2018

So somehow the behavior switched, and now @classproperty won't use the instance field, if the instance has a field shadowing a class-level field. Looking into why now.

cosmicexplorer added some commits Jun 5, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 5, 2018

I bisected and the difference literally appears to be because at one commit it returns a string and another an int (and I have isolated it to that). Looking into why that would cause this wild change in behavior.

Rewrite testing and docstrings for the correct behavior
clarify the result when an instance-level field or method shadows a class-level
version
@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 5, 2018

Ok, so I was able to isolate the different behavior in a previous commit, but wasn't able to repro the old behavior on the current commit, so I added testing and was able to remove the "caveats" section in the docstring. I have given up on trying to figure out why that was previously occurring. Now, if a class has an instance-level field _value that shadows a class-level field _value, it will use the class-level field if you call a classmethod or access a classproperty on an instance of the class.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 6, 2018

CI is green and this should be ready to go, please take another look. Let me know if I should be adding docstrings to the added methods in memo.py.

@@ -31,9 +31,183 @@ def method(self):
AbstractMethod()


class SingletonTest(unittest.TestCase):
class SingletonTest(TestBase):

This comment has been minimized.

@stuhood

stuhood Jun 6, 2018

Member

For the record, it's totally kosher to extend unittest.TestCase (and a good signal to a reader) if you don't have need of a pants workspace. Not a big deal either way (and not worth another CI run).

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 6, 2018

Contributor

Noted! I can totally understand that distinction.

@stuhood

stuhood approved these changes Jun 6, 2018

@cosmicexplorer cosmicexplorer merged commit efef00a into pantsbuild:master Jun 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment