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
EmailMessage.is_attachment should be a method #65290
Comments
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? |
Patch to change message.is_attachment from a property to a normal method. I've updated the doc and all calls to is_attachment. |
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. |
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. |
The alternative is to make EmailMessage.is_multipart a property. |
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. |
Here is a patch with more soft transition. Message.is_multipart() still works but emits deprecation warning. |
Based on the provisional API status, a faster deprecation plan could be to |
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. |
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. |
New changeset a3df1c24d586 by R David Murray in branch '3.4': New changeset f7aff40609e7 by R David Murray in branch 'default': |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: