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

mailbox.Mailbox does odd hasattr() check #64928

Closed
Rosuav opened this issue Feb 22, 2014 · 8 comments
Closed

mailbox.Mailbox does odd hasattr() check #64928

Rosuav opened this issue Feb 22, 2014 · 8 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@Rosuav
Copy link
Contributor

Rosuav commented Feb 22, 2014

BPO 20729
Nosy @warsaw, @terryjreedy, @bitdancer, @akheron, @Rosuav, @serhiy-storchaka
Files
  • mailbox_iters.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/serhiy-storchaka'
    closed_at = <Date 2014-08-13.07:00:11.644>
    created_at = <Date 2014-02-22.12:45:39.023>
    labels = ['type-bug', 'library']
    title = 'mailbox.Mailbox does odd hasattr() check'
    updated_at = <Date 2014-08-13.07:00:11.644>
    user = 'https://github.com/Rosuav'

    bugs.python.org fields:

    activity = <Date 2014-08-13.07:00:11.644>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2014-08-13.07:00:11.644>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2014-02-22.12:45:39.023>
    creator = 'Rosuav'
    dependencies = []
    files = ['34189']
    hgrepos = []
    issue_num = 20729
    keywords = ['patch']
    message_count = 8.0
    messages = ['211920', '211939', '211951', '211959', '211960', '225242', '225244', '225256']
    nosy_count = 8.0
    nosy_names = ['barry', 'terry.reedy', 'peter.otten', 'r.david.murray', 'python-dev', 'petri.lehtinen', 'Rosuav', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue20729'
    versions = ['Python 3.4', 'Python 3.5']

    @Rosuav
    Copy link
    Contributor Author

    Rosuav commented Feb 22, 2014

    Only noticed because I was searching the stdlib for hasattr calls, but in mailbox.Mailbox.update(), a check is done thus:

            if hasattr(arg, 'iteritems'):
                source = arg.items()
            elif hasattr(arg, 'items'):
                source = arg.items()
            else:
                source = arg

    If this is meant to support Python 2, it should probably use iteritems() in the first branch, but for Python 3, it's probably simpler to just drop the first check altogether:

            if hasattr(arg, 'items'):
                source = arg.items()
            else:
                source = arg

    Or possibly switch to EAFP:

        try:
            source = arg.items()
        except AttributeError:
            source = arg
    

    @serhiy-storchaka
    Copy link
    Member

    Actually it should be iteritems(). This is meant to support Mailbox, which has both iteritems() and items() methods. iteritems() returns an iterator and items() returns a list. Looks as changeset f340cb045bf9 was incorrectly applied to mailbox. Here is a patch which partially reverts changeset f340cb045bf9 for the mailbox module and fixes tests in 3.x.

    Perhaps we should change items() to return an iterator in Python 4.0.

    @serhiy-storchaka serhiy-storchaka added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 22, 2014
    @terryjreedy
    Copy link
    Member

    You have opened the door on a slight mess. The mailbox module provides a set + dict interface to on-disk mailbax files in various formats. The hg annotate and revision history commands indicate that most of 3.4 mailbox is unchanged since the trunk version was merged into py3k on 2006-5-27 in rev38453. However, on 2007-2-11 Guido changed .iterxxx to .xxx throughout the stdlib in rev40809.

    The bug you note is a side-effect of this patch. It overall left mailbax in a somewhat inconsistent state as it did *not* update the mailbox dict API by removing the mailbox.iterkeys/values/items methods and replacing the .keys/values/items methods. As a result, the mailbox methods that space efficiently iterated thru self.iterkeys now iterate through self.keys == list(self.iterkeys). Example:
    def clear(self):
    """Delete all messages."""
    for key in self.keys(): # was self.iterkeys()
    self.discard(key)

    To fix this, I think we should either

    1. revert the rev40809 changes to mailbox, including in the line you point to, or

    2. complete the rev40809 changes by updating to a Py3 interface, and make the change you suggest.

    3. is much easier, but the API looks odd to a py3-only programmer.
      After writing this message, I see that Serhiy wrote a patch to do this.

    4. is an api change that perhaps should have happened in 3.0. Deprecation is awkward since people should not change from, for instance, self.iterkeys to self.key, until the api change in made.

    @peterotten
    Copy link
    Mannequin

    peterotten mannequin commented Feb 22, 2014

    Do you expect many use cases that rely on items(), keys(), and values() being lists?
    Maybe it would be acceptable to make these lazy in 3.5, but keep the iterXXX() variants as aliases indefinitely.

    @serhiy-storchaka
    Copy link
    Member

    Note that there is a difference between Mailbox and dict interface: __iter__()
    iterates over values, not keys.

    clear() should use keys(), not iterkeys(), because it modifies iterated dict.

    @serhiy-storchaka
    Copy link
    Member

    If there are no objections I'll commit this patch.

    @serhiy-storchaka serhiy-storchaka self-assigned this Aug 12, 2014
    @bitdancer
    Copy link
    Member

    Committing the patch seems like the right thing to do at this point in time.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 13, 2014

    New changeset 5fd1f8271e8a by Serhiy Storchaka in branch '3.4':
    Issue bpo-20729: Restored the use of lazy iterkeys()/itervalues()/iteritems()
    http://hg.python.org/cpython/rev/5fd1f8271e8a

    New changeset acb30ed7eceb by Serhiy Storchaka in branch 'default':
    Issue bpo-20729: Restored the use of lazy iterkeys()/itervalues()/iteritems()
    http://hg.python.org/cpython/rev/acb30ed7eceb

    @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
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants