Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(106318)

#11610: Improving property to accept abstract methods

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 8 months ago by dsdale24
Modified:
7 years, 11 months ago
Reviewers:
benjamin, ncoghlan
CC:
Nick Coghlan, Benjamin Peterson, stutzbach, eric.araujo, durban, dsdale24, devnull_psf.upfronthosting.co.za, eric.snow, dsdale24, stutzbach, devnull_psf.upfronthosting.co.za
Visibility:
Public.

Patch Set 1 #

Total comments: 16

Patch Set 2 #

Patch Set 3 #

Total comments: 4

Patch Set 4 #

Patch Set 5 #

Patch Set 6 #

Patch Set 7 #

Total comments: 6

Patch Set 8 #

Total comments: 32

Patch Set 9 #

Total comments: 10

Patch Set 10 #

Total comments: 30

Patch Set 11 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/abc.rst View 5 chunks +60 lines, -15 lines 0 comments Download
Doc/whatsnew/3.3.rst View 5 6 7 8 9 10 1 chunk +17 lines, -0 lines 0 comments Download
Include/object.h View 10 1 chunk +1 line, -0 lines 0 comments Download
Lib/abc.py View 1 2 3 4 5 6 7 8 9 10 6 chunks +16 lines, -3 lines 0 comments Download
Lib/numbers.py View 5 6 7 8 9 10 4 chunks +9 lines, -5 lines 0 comments Download
Lib/test/test_abc.py View 1 2 3 4 5 6 7 8 9 10 5 chunks +162 lines, -10 lines 0 comments Download
Lib/test/test_property.py View 8 9 10 1 chunk +23 lines, -0 lines 0 comments Download
Misc/ACKS View 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
Objects/descrobject.c View 1 2 7 8 9 10 2 chunks +38 lines, -1 line 0 comments Download
Objects/funcobject.c View 7 8 9 10 4 chunks +44 lines, -2 lines 0 comments Download
Objects/object.c View 10 1 chunk +23 lines, -0 lines 0 comments Download

Messages

Total messages: 42
durban
I looked at the patch (I didn't tested it). Here are my comments at first ...
8 years, 8 months ago #1
dsdale24
Thank you for the feedback. The reason I suggested deprecating abstractproperty is that I think ...
8 years, 8 months ago #2
gvanrossum
http://bugs.python.org/review/11610/diff/2175/4967 File Lib/abc.py (right): http://bugs.python.org/review/11610/diff/2175/4967#newcode81 Lib/abc.py:81: @abstractmethod You can't just change the semantics of @abstractproperty ...
8 years, 8 months ago #3
dsdale24
I think Guido's concerns are already addressed in the existing code, but may require a ...
8 years, 8 months ago #4
gvanrossum
Ok, I misunderstood your code and was misled by your changes to the example docs. ...
8 years, 8 months ago #5
dsdale24
http://bugs.python.org/review/11610/diff/2175/4967 File Lib/abc.py (right): http://bugs.python.org/review/11610/diff/2175/4967#newcode121 Lib/abc.py:121: return abstractproperty.create_property(fun, self.fset, self.fdel, I think the calls in ...
8 years, 8 months ago #6
durban
http://bugs.python.org/review/11610/diff/2864/7421 File Objects/descrobject.c (right): http://bugs.python.org/review/11610/diff/2864/7421#newcode1340 Objects/descrobject.c:1340: Py_DECREF(isabstract); It's probably not a good idea to call ...
8 years, 5 months ago #7
dsdale24
http://bugs.python.org/review/11610/diff/2864/7421 File Objects/descrobject.c (right): http://bugs.python.org/review/11610/diff/2864/7421#newcode1341 Objects/descrobject.c:1341: return PyObject_IsTrue(isabstract); On 2011/06/20 22:40:40, durban wrote: > Also, ...
8 years, 3 months ago #8
durban
http://bugs.python.org/review/11610/diff/2864/7421 File Objects/descrobject.c (right): http://bugs.python.org/review/11610/diff/2864/7421#newcode1341 Objects/descrobject.c:1341: return PyObject_IsTrue(isabstract); On 2011/07/22 16:01:01, dsdale24 wrote: > On ...
8 years, 3 months ago #9
Benjamin Peterson
http://bugs.python.org/review/11610/diff/3073/8119 File Doc/library/abc.rst (right): http://bugs.python.org/review/11610/diff/3073/8119#newcode182 Doc/library/abc.rst:182: .. note:: I don't think this should be in ...
7 years, 11 months ago #10
gvanrossum
http://bugs.python.org/review/11610/diff/3073/8119 File Doc/library/abc.rst (right): http://bugs.python.org/review/11610/diff/3073/8119#newcode157 Doc/library/abc.rst:157: def my_abstract_method(self, ...): my_abstract_classmethod http://bugs.python.org/review/11610/diff/3073/8119#newcode161 Doc/library/abc.rst:161: def my_abstract_method(self, ...): ...
7 years, 11 months ago #11
dsdale24
I have a few follow-up questions relating to the comments. Also, I overlooked that some ...
7 years, 11 months ago #12
Benjamin Peterson
http://bugs.python.org/review/11610/diff/3073/8121 File Lib/abc.py (right): http://bugs.python.org/review/11610/diff/3073/8121#newcode73 Lib/abc.py:73: import warnings On 2011/11/29 17:20:36, dsdale24 wrote: > On ...
7 years, 11 months ago #13
dsdale24
Overlooked one comment in my previous response. http://bugs.python.org/review/11610/diff/3073/8123 File Lib/test/test_abc.py (right): http://bugs.python.org/review/11610/diff/3073/8123#newcode20 Lib/test/test_abc.py:20: self.assertFalse(getattr(bar, "__isabstractmethod__", ...
7 years, 11 months ago #14
dsdale24
http://bugs.python.org/review/11610/diff/3073/8127 File Objects/descrobject.c (right): http://bugs.python.org/review/11610/diff/3073/8127#newcode1348 Objects/descrobject.c:1348: propertyobject *prop = (propertyobject *)self; On 2011/11/29 17:27:46, Benjamin ...
7 years, 11 months ago #15
Benjamin Peterson
On 2011/11/29 19:24:13, dsdale24 wrote: > http://bugs.python.org/review/11610/diff/3073/8127 > File Objects/descrobject.c (right): > > http://bugs.python.org/review/11610/diff/3073/8127#newcode1348 > ...
7 years, 11 months ago #16
dsdale24
http://bugs.python.org/review/11610/diff/3073/8127 File Objects/descrobject.c (right): http://bugs.python.org/review/11610/diff/3073/8127#newcode1335 Objects/descrobject.c:1335: PyObject* isabstract = PyObject_GetAttrString(obj, "__isabstractmethod__"); On 2011/11/29 02:45:54, Benjamin ...
7 years, 11 months ago #17
Benjamin Peterson
On 2011/11/29 19:28:03, dsdale24 wrote: > http://bugs.python.org/review/11610/diff/3073/8127 > File Objects/descrobject.c (right): > > http://bugs.python.org/review/11610/diff/3073/8127#newcode1335 > ...
7 years, 11 months ago #18
dsdale24
On 2011/11/29 19:25:46, Benjamin Peterson wrote: > On 2011/11/29 19:24:13, dsdale24 wrote: > > http://bugs.python.org/review/11610/diff/3073/8127 ...
7 years, 11 months ago #19
dsdale24
On 2011/11/29 19:29:32, Benjamin Peterson wrote: > On 2011/11/29 19:28:03, dsdale24 wrote: > > http://bugs.python.org/review/11610/diff/3073/8127 ...
7 years, 11 months ago #20
Benjamin Peterson
Getting closer! http://bugs.python.org/review/11610/diff/3736/11885 File Objects/descrobject.c (right): http://bugs.python.org/review/11610/diff/3736/11885#newcode1339 Objects/descrobject.c:1339: _Py_IDENTIFIER(__isabstractmethod__); Needs to go at beginning of ...
7 years, 11 months ago #21
dsdale24
http://bugs.python.org/review/11610/diff/3736/11885 File Objects/descrobject.c (right): http://bugs.python.org/review/11610/diff/3736/11885#newcode1340 Objects/descrobject.c:1340: PyObject* isabstract = _PyObject_GetAttrId(obj, &PyId___isabstractmethod__); On 2011/12/04 22:56:35, Benjamin ...
7 years, 11 months ago #22
dsdale24
http://bugs.python.org/review/11610/diff/3736/11885 File Objects/descrobject.c (right): http://bugs.python.org/review/11610/diff/3736/11885#newcode1344 Objects/descrobject.c:1344: return 0; On 2011/12/04 22:56:35, Benjamin Peterson wrote: > ...
7 years, 11 months ago #23
Benjamin Peterson
On 2011/12/05 15:10:36, dsdale24 wrote: > http://bugs.python.org/review/11610/diff/3736/11885 > File Objects/descrobject.c (right): > > http://bugs.python.org/review/11610/diff/3736/11885#newcode1344 > ...
7 years, 11 months ago #24
Benjamin Peterson
http://bugs.python.org/review/11610/diff/3736/11885 File Objects/descrobject.c (right): http://bugs.python.org/review/11610/diff/3736/11885#newcode1340 Objects/descrobject.c:1340: PyObject* isabstract = _PyObject_GetAttrId(obj, &PyId___isabstractmethod__); On 2011/12/05 14:59:17, dsdale24 ...
7 years, 11 months ago #25
dsdale24
I just noticed that the check_isabstract function only needs to take the function as input, ...
7 years, 11 months ago #26
Nick Coghlan
The core functionality changes look fine to me, I just have a number of cleanup ...
7 years, 11 months ago #27
dsdale24
http://bugs.python.org/review/11610/diff/3760/12226 File Objects/descrobject.c (right): http://bugs.python.org/review/11610/diff/3760/12226#newcode1336 Objects/descrobject.c:1336: if (res == -1) On 2011/12/06 09:14:01, Nick Coghlan ...
7 years, 11 months ago #28
dsdale24
http://bugs.python.org/review/11610/diff/3760/12218 File Doc/library/abc.rst (right): http://bugs.python.org/review/11610/diff/3760/12218#newcode184 Doc/library/abc.rst:184: example, Python's built-in property does the equivalent of:: On ...
7 years, 11 months ago #29
Benjamin Peterson
On 2011/12/06 09:14:01, Nick Coghlan wrote: > http://bugs.python.org/review/11610/diff/3760/12227#newcode821 > Objects/funcobject.c:821: if (res == -1) > ...
7 years, 11 months ago #30
dsdale24
http://bugs.python.org/review/11610/diff/3760/12218 File Doc/library/abc.rst (right): http://bugs.python.org/review/11610/diff/3760/12218#newcode184 Doc/library/abc.rst:184: example, Python's built-in property does the equivalent of:: On ...
7 years, 11 months ago #31
dsdale24
http://bugs.python.org/review/11610/diff/3760/12221 File Lib/abc.py (right): http://bugs.python.org/review/11610/diff/3760/12221#newcode23 Lib/abc.py:23: @classmethod On 2011/12/06 09:14:01, Nick Coghlan wrote: > This ...
7 years, 11 months ago #32
dsdale24
http://bugs.python.org/review/11610/diff/3760/12223 File Lib/test/test_abc.py (right): http://bugs.python.org/review/11610/diff/3760/12223#newcode20 Lib/test/test_abc.py:20: self.assertFalse(getattr(bar, "__isabstractmethod__", False)) On 2011/12/06 09:14:01, Nick Coghlan wrote: ...
7 years, 11 months ago #33
Nick Coghlan
On 2011/12/06 17:45:58, dsdale24 wrote: > The original test wants to see that bar does ...
7 years, 11 months ago #34
Nick Coghlan
On 2011/12/06 16:33:49, Benjamin Peterson wrote: > I think it's fine with out braces as ...
7 years, 11 months ago #35
Nick Coghlan
On 2011/12/06 17:25:32, dsdale24 wrote: > I respectfully disagree. This is the only place in ...
7 years, 11 months ago #36
Nick Coghlan
On 2011/12/06 17:07:36, dsdale24 wrote: > How about: > > In order to correctly interoperate ...
7 years, 11 months ago #37
Benjamin Peterson
On 2011/12/07 01:04:57, Nick Coghlan wrote: > On 2011/12/06 16:33:49, Benjamin Peterson wrote: > > ...
7 years, 11 months ago #38
Nick Coghlan
On 2011/12/07 02:03:05, Benjamin Peterson wrote: > > PEP 7 says otherwise. > > Where? ...
7 years, 11 months ago #39
Benjamin Peterson
On 2011/12/07 13:30:05, Nick Coghlan wrote: > On 2011/12/07 02:03:05, Benjamin Peterson wrote: > > ...
7 years, 11 months ago #40
Nick Coghlan
On 2011/12/07 15:16:26, Benjamin Peterson wrote: > It doesn't say anything about K&R. It just ...
7 years, 11 months ago #41
Benjamin Peterson
7 years, 11 months ago #42
On 2011/12/07 16:24:19, Nick Coghlan wrote:
> On 2011/12/07 15:16:26, Benjamin Peterson wrote:
> > It doesn't say anything about K&R. It just has the example, which is reading
a
> > lot into it.
> 
> Um, the example *is* K&R style. How much more explicit do you want it to get?

Uh, maybe saying K&R? At any rate, this isn't worth arguing about, so sure put
the braces back in.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+