classification
Title: EmailMessage.is_attachment should be a method
Type: behavior Stage: resolved
Components: email Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: r.david.murray Nosy List: barry, brandon-rhodes, joegod, merwok, ncoghlan, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2014-03-29 01:26 by brandon-rhodes, last changed 2014-09-20 22:18 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
attach_not_property.patch joegod, 2014-08-04 06:35 review
multipart_is_property.patch joegod, 2014-08-04 11:15 review
multipart_is_property_2.patch serhiy.storchaka, 2014-08-04 12:21 Softer transition review
is_attachment_as_method.patch r.david.murray, 2014-09-20 21:00 review
Messages (11)
msg215104 - (view) Author: Brandon Rhodes (brandon-rhodes) * Date: 2014-03-29 01:26
I love properties and think they should be everywhere. But consistency is more important, so I suspect that EmailMessage.is_attachment should be demoted to a normal method. Why? Because if it remains a property then I am likely to first write:

    if msg.is_attachment:
        ...

and then later, when doing another bit of email logic, write:

    if msg.is_multipart:
        ...

Unfortunately this second piece of code will give me no error and will appear to run just fine, because bool(a_method) always returns True without a problem or warning or error. But the result will not be what I expect: the if statement's true block will always run, regardless of whether the message is multipart.

Since EmailMessage is still provisional, and since no one can use is_attachment yet anyway because it is broken for nearly all attachments, mightn't we make these two features consistent before calling it official?
msg224688 - (view) Author: Joseph Godbehere (joegod) * Date: 2014-08-04 06:35
Patch to change message.is_attachment from a property to a normal method. I've updated the doc and all calls to is_attachment.
msg224689 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-08-04 06:40
This is your call David - I agree consistency is highly desirable, and having a chance to find and fix this kind of discrepancy is a large part of why we introduced provisional APIs.
msg224693 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-08-04 07:21
Unfortunately this will silently break existing code because msg.is_attachment will be always true. But it is possible to make EmailMessage.is_attachment a property which returns special callable with the __bool__() method which will emit deprecation warning and call the __call__() method. After some intermediate period (one or two releases) it can be replaced by regular method.
msg224695 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-08-04 07:24
The alternative is to make EmailMessage.is_multipart a property.
msg224708 - (view) Author: Joseph Godbehere (joegod) * Date: 2014-08-04 11:15
Very good point, Serhiy.

Here is an alternative patch, which instead changes Message.is_multipart from a method to a property as per your suggestion. This way incorrect usage should fail noisily.

This patch is against the relevant docs, tests, is_multipart and calls to is_multipart only.
msg224711 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-08-04 12:21
Here is a patch with more soft transition. Message.is_multipart() still works but emits deprecation warning.
msg224712 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-08-04 12:25
Based on the provisional API status, a faster deprecation plan could be to
do Serhiy's patch in a 3.4 maintenance release and the hard break in 3.5
msg224869 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-08-05 18:19
is_multipart is *not* part of the provisional API, though; only is_attachment is.

So per my understanding of the provisional rules, we should either make is_attachment a method in both 3.4 maint and 3.5, or make is_multipart emit a deprecation warning in 3.5.  

I lean toward the former for backward compatibility reasons, with Serhiy's addition of making it emit a warning if not called in 3.4.
msg227175 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-09-20 21:00
Here is a patch.  Sorry for leaving it until the last minute...maybe someone can review it, but it is simple enough I'll commit it soon regardless.
msg227184 - (view) Author: Roundup Robot (python-dev) Date: 2014-09-20 22:17
New changeset a3df1c24d586 by R David Murray in branch '3.4':
#21091: make is_attachment a method.
https://hg.python.org/cpython/rev/a3df1c24d586

New changeset f7aff40609e7 by R David Murray in branch 'default':
Merge: #21091: make is_attachment a method.
https://hg.python.org/cpython/rev/f7aff40609e7
History
Date User Action Args
2014-09-20 22:18:04r.david.murraysetstatus: open -> closed
type: behavior
stage: needs patch -> resolved
resolution: fixed
versions: + Python 3.4
2014-09-20 22:17:26python-devsetnosy: + python-dev
messages: + msg227184
2014-09-20 21:00:36r.david.murraysetfiles: + is_attachment_as_method.patch

messages: + msg227175
2014-08-06 08:56:28serhiy.storchakasetstage: commit review -> needs patch
2014-08-05 18:19:20r.david.murraysetmessages: + msg224869
2014-08-04 12:25:11ncoghlansetmessages: + msg224712
2014-08-04 12:21:21serhiy.storchakasetfiles: + multipart_is_property_2.patch

messages: + msg224711
2014-08-04 11:15:55joegodsetfiles: + multipart_is_property.patch

messages: + msg224708
2014-08-04 07:24:48serhiy.storchakasetmessages: + msg224695
2014-08-04 07:21:46serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg224693
2014-08-04 06:40:02ncoghlansetnosy: + ncoghlan
messages: + msg224689

assignee: r.david.murray
stage: commit review
2014-08-04 06:35:54joegodsetfiles: + attach_not_property.patch

nosy: + joegod
messages: + msg224688

keywords: + patch
2014-04-04 21:23:39merwoksetnosy: + merwok

versions: + Python 3.5, - Python 3.4
2014-03-29 01:26:19brandon-rhodescreate