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

EmailMessage.is_attachment should be a method #65290

Closed
brandon-rhodes mannequin opened this issue Mar 29, 2014 · 11 comments
Closed

EmailMessage.is_attachment should be a method #65290

brandon-rhodes mannequin opened this issue Mar 29, 2014 · 11 comments
Assignees
Labels
topic-email type-bug An unexpected behavior, bug, or error

Comments

@brandon-rhodes
Copy link
Mannequin

brandon-rhodes mannequin commented Mar 29, 2014

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
    Copy link
    Mannequin Author

    brandon-rhodes mannequin commented Mar 29, 2014

    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?

    @brandon-rhodes brandon-rhodes mannequin added the topic-email label Mar 29, 2014
    @joegod
    Copy link
    Mannequin

    joegod mannequin commented Aug 4, 2014

    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
    Copy link
    Contributor

    ncoghlan commented Aug 4, 2014

    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
    Copy link
    Member

    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
    Copy link
    Member

    The alternative is to make EmailMessage.is_multipart a property.

    @joegod
    Copy link
    Mannequin

    joegod mannequin commented Aug 4, 2014

    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
    Copy link
    Member

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

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Aug 4, 2014

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Mannequin

    python-dev mannequin commented Sep 20, 2014

    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

    @bitdancer bitdancer added the type-bug An unexpected behavior, bug, or error label Sep 20, 2014
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-email type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants