EmailMessage.is_attachment should be a method

[brandon-rhodes]

BPO	21091
Nosy	@warsaw, @ncoghlan, @merwok, @bitdancer, @serhiy-storchaka
Files	attach_not_property.patch multipart_is_property.patch multipart_is_property_2.patch: Softer transition is_attachment_as_method.patch

^{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 = 'https://github.com/bitdancer'
closed_at = <Date 2014-09-20.22:18:04.333>
created_at = <Date 2014-03-29.01:26:19.015>
labels = ['type-bug', 'expert-email']
title = 'EmailMessage.is_attachment should be a method'
updated_at = <Date 2014-09-20.22:18:04.331>
user = 'https://bugs.python.org/brandon-rhodes'

bugs.python.org fields:

activity = <Date 2014-09-20.22:18:04.331>
actor = 'r.david.murray'
assignee = 'r.david.murray'
closed = True
closed_date = <Date 2014-09-20.22:18:04.333>
closer = 'r.david.murray'
components = ['email']
creation = <Date 2014-03-29.01:26:19.015>
creator = 'brandon-rhodes'
dependencies = []
files = ['36243', '36249', '36250', '36670']
hgrepos = []
issue_num = 21091
keywords = ['patch']
message_count = 11.0
messages = ['215104', '224688', '224689', '224693', '224695', '224708', '224711', '224712', '224869', '227175', '227184']
nosy_count = 8.0
nosy_names = ['barry', 'ncoghlan', 'eric.araujo', 'r.david.murray', 'brandon-rhodes', 'python-dev', 'serhiy.storchaka', 'joegod']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue21091'
versions = ['Python 3.4', 'Python 3.5']

[brandon-rhodes]

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?

[joegod]

Patch to change message.is_attachment from a property to a normal method. I've updated the doc and all calls to is_attachment.

[ncoghlan]

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.

[serhiy-storchaka]

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.

[serhiy-storchaka]

The alternative is to make EmailMessage.is_multipart a property.

[joegod]

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.

[serhiy-storchaka]

Here is a patch with more soft transition. Message.is_multipart() still works but emits deprecation warning.

[ncoghlan]

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

[bitdancer]

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.

[bitdancer]

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.

[python-dev]

New changeset a3df1c24d586 by R David Murray in branch '3.4':
bpo-21091: make is_attachment a method.
https://hg.python.org/cpython/rev/a3df1c24d586

New changeset f7aff40609e7 by R David Murray in branch 'default':
Merge: bpo-21091: make is_attachment a method.
https://hg.python.org/cpython/rev/f7aff40609e7