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

order of decorators @abstractmethod and @classmethod is significant (is not documented to be in @abstractclassmethod which advises their combined use) #60471

Closed
christopherthemagnificent mannequin opened this issue Oct 17, 2012 · 12 comments
Labels
docs Documentation in the Doc dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@christopherthemagnificent
Copy link
Mannequin

BPO 16267
Nosy @ncoghlan, @benjaminp, @merwok, @bitdancer, @asvetlov, @durban, @ericsnowcurrently

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2012-12-09.05:01:11.625>
created_at = <Date 2012-10-17.15:41:20.991>
labels = ['interpreter-core', 'type-bug', 'docs']
title = 'order of decorators @abstractmethod and @classmethod is significant (is not documented to be in @abstractclassmethod which advises their combined use)'
updated_at = <Date 2012-12-09.05:01:11.625>
user = 'https://bugs.python.org/christopherthemagnificent'

bugs.python.org fields:

activity = <Date 2012-12-09.05:01:11.625>
actor = 'ncoghlan'
assignee = 'docs@python'
closed = True
closed_date = <Date 2012-12-09.05:01:11.625>
closer = 'ncoghlan'
components = ['Documentation', 'Interpreter Core']
creation = <Date 2012-10-17.15:41:20.991>
creator = 'christopherthemagnificent'
dependencies = []
files = []
hgrepos = []
issue_num = 16267
keywords = []
message_count = 12.0
messages = ['173183', '173184', '173188', '173189', '173190', '173198', '173200', '173280', '173282', '175417', '176823', '177159']
nosy_count = 14.0
nosy_names = ['ncoghlan', 'benjamin.peterson', 'stutzbach', 'eric.araujo', 'Arfrever', 'r.david.murray', 'asvetlov', 'daniel.urban', 'christopherthemagnificent', 'docs@python', 'dsdale24', 'python-dev', 'eric.snow', 'Darren.Dale']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue16267'
versions = ['Python 3.3', 'Python 3.4']

@christopherthemagnificent
Copy link
Mannequin Author

This may be an issue with the interpreter behavior or it may be a documentation issue.

Note: I only selected Python 3.3 as the version, but it probably affects MANY other Python versions.

Python 3.3.0 (v3.3.0:bd8afb90ebf2, Sep 29 2012, 01:25:11) 
[GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
Type "copyright", "credits" or "license()" for more information.
>>> import abc
>>> help(abc.abstractclassmethod)
Help on class abstractclassmethod in module abc:
class abstractclassmethod(builtins.classmethod)
 |  A decorator indicating abstract classmethods.
 |  
 |  Similar to abstractmethod.
 |  
 |  Usage:
 |  
 |      class C(metaclass=ABCMeta):
 |          @abstractclassmethod
 |          def my_abstract_classmethod(cls, ...):
 |              ...
 |  
 |  'abstractclassmethod' is deprecated. Use 'classmethod' with
 |  'abstractmethod' instead.
. (et cetra)
.
.
>>> # doesn't work
>>> class Demo(metaclass=abc.ABCMeta):
... 	@abc.abstractmethod
... 	@classmethod
... 	def test(cls):
... 		pass
... 
Traceback (most recent call last):
  File "<pyshell#26>", line 1, in <module>
    class Demo3(metaclass=abc.ABCMeta):
  File "<pyshell#26>", line 3, in Demo3
    @classmethod
  File "/Library/Frameworks/Python.framework/Versions/3.3/lib/python3.3/abc.py", line 24, in abstractmethod
    funcobj.__isabstractmethod__ = True
AttributeError: attribute '__isabstractmethod__' of 'classmethod' objects is not writable
>>> # DOES work
>>> class Demo2(metaclass=abc.ABCMeta):
... 	@classmethod
... 	@abc.abstractmethod
... 	def test(cls):
... 		pass
... 	
>>> Demo2()
Traceback (most recent call last):
  File "<pyshell#33>", line 1, in <module>
    Demo4()
TypeError: Can't instantiate abstract class Demo2 with abstract methods test

Hopefully this is enough documentation to show what the issues is. If not, just chime in. :-)

@christopherthemagnificent christopherthemagnificent mannequin added docs Documentation in the Doc dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Oct 17, 2012
@asvetlov
Copy link
Contributor

I think better to fix code to make first sample also work.
It can be done as special cases in abc.abstractmethod to process classmethod/staticmethod objects properly.

@bitdancer
Copy link
Member

I've added the nosy list from bpo-11610, in case complicating the implementation is seen as sub-optimal :)

@benjaminp
Copy link
Contributor

I don't see why classmethod/staticmethod should be special cased if it doesn't work for other decorators.

@dsdale24
Copy link
Mannequin

dsdale24 mannequin commented Oct 17, 2012

Quoting the documentation for abstractmethod:

"When abstractmethod() is applied in combination with other method descriptors, it should be applied as the innermost decorator, as shown in the following usage examples:"

The examples include staticmethod and classmethod.

@christopherthemagnificent
Copy link
Mannequin Author

As Darren Dale pointed out, it looks like this is a (partial) documentation issue. I think it's plausible that someone like me, who has used abstractmethod by itself, would read the docs for abstractclassmethod and not re-read the docs on abstract method to know that he needs to put the one decorator first and other other second.

Changing Python to make it indifferent to the order of classmethod and abstractmethod wouldn't be a bad idea if it isn't too hairy to implement, since it does not seem to be intuitive to me and probably others that the order of the decorators in this specific situation should matter.

At bare minimum, I recommend that the documentation for abstractclassmethod and abstractstaticmethod should be updated to indicate not merely that abstractmethod and either classmethod or staticmethod should be used together, but IN WHICH ORDER they should be used, if it is decided to preserve the sensitivity to ordering.

:-)

@asvetlov
Copy link
Contributor

After brief looking sources I figured out it can be solved by adding setters for __isabstractmethod__ to classmethod/staticmethod objects.
It can be done, I'll try to make a patch.

For property situation is worse: property is abstract if any of getter/setter/deleter is abstract.
Which object attribute should be set for setting __isabstractmethod__ for property?

We can make the rule: abstractmethod for classmethod/staticmethod/property should set descriptor as abstract, not functions behind it.

I'm not sure is it true solution but I like to try to make a patch for that.

Anyway, the patch for describing current behavior in the docs is welcome.

@ericsnowcurrently
Copy link
Member

The catch is that when abstractmethod is the inner decorator, __isabstractmethod__ is set on the object that classmethod/staticmethod is wrapping. When abstractmethod is the outer decorator, __isabstractmethod__ is set on the resulting classmethod/staticmethod object instead. Unless there is some practical reason that the distinction matters, I'm +1 on letting __isabstractmethod__ be set on classmethods and staticmethods.

@DarrenDale
Copy link
Mannequin

DarrenDale mannequin commented Oct 18, 2012

There is a very practical reason, which was the whole point of bpo-11610. Descriptors are should declare themselves abstract when they are composed of abstract methods. If you have a property with an concrete getter but an abstract setter, the property should declare itself abstract until such time as it is provided a concrete setter. If we allow __isabstractmethod__ to be settable by @AbstractMethod, it undermines the whole scheme of descriptors delegating their abstractedness to the methods of which they are composed.

@ncoghlan
Copy link
Contributor

It took me a while to get my brain back up to speed with the full rationale behind the current design (mostly by rereading the multitude of comment on bpo-11610).

As Darren says, the main advantage of the current scheme is that the wrapper descriptors deliberately *don't* have any concept of abstract/non-abstract independent of the methods that make them up. So I think the main thing to do is change the documentation of the affected descriptors to be more explicit about the required order of the replacement decorators. Otherwise people are likely to map "abstractXmethod" to "@AbstractMethod + @xmethod" and write them in that order (which won't work).

@asvetlov
Copy link
Contributor

asvetlov commented Dec 3, 2012

After trying to make patch I've realized — better to leave current behavior as is and change documentation only.

@python-dev
Copy link
Mannequin

python-dev mannequin commented Dec 8, 2012

New changeset 3345afd6dc61 by Nick Coghlan in branch '3.3':
Close issue bpo-16267: better docs for @AbstractMethod composition
http://hg.python.org/cpython/rev/3345afd6dc61

New changeset be7202c38089 by Nick Coghlan in branch 'default':
Merge from 3.3 (issue bpo-16267)
http://hg.python.org/cpython/rev/be7202c38089

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants