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

gh-55454: Add IMAP4 IDLE support to imaplib #122542

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

foresto
Copy link

@foresto foresto commented Aug 1, 2024

This extends imaplib with support for the rfc2177 IMAP IDLE command, as requested in #55454. It allows events to be pushed to a client as they occur, rather than having to continually poll for mailbox changes.

The interface is a new idle() method, which returns an iterable context manager. Entering the context starts IDLE mode, during which events (untagged responses) can be retrieved using the iteration protocol. Exiting the context sends DONE to the server, ending IDLE mode.

An optional time limit for the IDLE session is supported, for use with servers that impose an inactivity timeout.

The context manager also offers a burst() method, designed for programs wishing to process events in batch rather than one at a time.

Notable differences from other implementations:

  • It's an extension to imaplib, rather than a replacement.
  • It doesn't introduce additional threads.
  • It doesn't impose new requirements on the use of imaplib's existing methods.
  • It passes the unit tests in CPython's test/test_imaplib.py module (and adds new ones).
  • It works on Windows, Linux, and other unix-like systems.
  • It makes IDLE available on all of imaplib's client variants (including IMAP4_stream).
  • The interface is pythonic and easy to use.

Caveats:

  • Due to a Windows limitation, the special case of IMAP4_stream running on Windows lacks a duration/timeout feature. (This is the stdin/stdout pipe connection variant; timeouts work fine for socket-based connections, even on Windows.) I have documented it where appropriate.

  • The file-like imaplib instance attributes are changed from buffered to unbuffered mode. This could potentially break any client code that uses those objects directly without expecting partial reads/writes. However, they are undocumented. As such, I think (and PEP 8 confirms) that they are fair game for changes. https://peps.python.org/pep-0008/#public-and-internal-interfaces

Usage examples:

#55454 (comment)

Recent discussion:

https://discuss.python.org/t/gauging-interest-in-my-imap4-idle-implementation-for-imaplib/59272

Earlier requests and suggestions:

#55454

https://mail.python.org/archives/list/[email protected]/thread/C4TVEYL5IBESQQPPS5GBR7WFBXCLQMZ2/


📚 Documentation preview 📚: https://cpython-previews--122542.org.readthedocs.build/

@foresto foresto requested a review from a team as a code owner August 1, 2024 03:13
Copy link

cpython-cla-bot bot commented Aug 1, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

This extends imaplib with support for the rfc2177 IMAP IDLE command,
as requested in python#55454.  It allows events to be pushed to a client as
they occur, rather than having to continually poll for mailbox changes.

The interface is a new idle() method, which returns an iterable context
manager.  Entering the context starts IDLE mode, during which events
(untagged responses) can be retrieved using the iteration protocol.
Exiting the context sends DONE to the server, ending IDLE mode.

An optional time limit for the IDLE session is supported, for use with
servers that impose an inactivity timeout.

The context manager also offers a burst() method, designed for programs
wishing to process events in batch rather than one at a time.

Notable differences from other implementations:

- It's an extension to imaplib, rather than a replacement.
- It doesn't introduce additional threads.
- It doesn't impose new requirements on the use of imaplib's existing methods.
- It passes the unit tests in CPython's test/test_imaplib.py module
  (and adds new ones).
- It works on Windows, Linux, and other unix-like systems.
- It makes IDLE available on all of imaplib's client variants
  (including IMAP4_stream).
- The interface is pythonic and easy to use.

Caveats:

- Due to a Windows limitation, the special case of IMAP4_stream running
  on Windows lacks a duration/timeout feature. (This is the stdin/stdout
  pipe connection variant; timeouts work fine for socket-based
  connections, even on Windows.) I have documented it where appropriate.

- The file-like imaplib instance attributes are changed from buffered to
  unbuffered mode. This could potentially break any client code that
  uses those objects directly without expecting partial reads/writes.
  However, these attributes are undocumented. As such, I think (and
  PEP 8 confirms) that they are fair game for changes.
  https://peps.python.org/pep-0008/#public-and-internal-interfaces

Usage examples:

python#55454 (comment)

Original discussion:

https://discuss.python.org/t/gauging-interest-in-my-imap4-idle-implementation-for-imaplib/59272

Earlier requests and suggestions:

python#55454

https://mail.python.org/archives/list/[email protected]/thread/C4TVEYL5IBESQQPPS5GBR7WFBXCLQMZ2/
@foresto
Copy link
Author

foresto commented Sep 1, 2024

Can someone find time for a code review?

Copy link
Contributor

@mcepl mcepl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I haven’t dived deep enough to the async stuff.

Doc/library/imaplib.rst Outdated Show resolved Hide resolved
- Add example idle response tuples, to make the minor difference from other
  imaplib response tuples more obvious.
- Merge the idle context manager's burst() method docs with the IMAP
  object's idle() method docs, for easier understanding.
- Upgrade the Windows note regarding lack of pipe timeouts to a warning.
- Rephrase various things for clarity.
@foresto
Copy link
Author

foresto commented Sep 18, 2024

@mcepl Did you see my last comment? Have you had a chance to finish reviewing?

Copy link
Contributor

@mcepl mcepl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is very appropriate.

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing, @foresto! With great PRs, comes great requests for changes 😉

Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Lib/test/test_imaplib.py Show resolved Hide resolved
Lib/imaplib.py Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Lib/test/test_imaplib.py Show resolved Hide resolved
Lib/imaplib.py Outdated Show resolved Hide resolved
gpshead and others added 5 commits September 21, 2024 10:39
Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Peter Bierma <[email protected]>
@gpshead gpshead self-requested a review September 21, 2024 17:50
@gvanrossum
Copy link
Member

@ZeroIntensity how well did you review this? If you think you understand the implementation and agree with it, I can rubber-stamp it. OTOH if @serhiy-storchaka is still planning to review this (as he indicated a few months ago) I'd rather wait for his opinion, he's extremely thorough.

@ZeroIntensity
Copy link
Member

My original review was pretty thorough, but I didn't do a second pass over once the requested changes had been addressed. I'll re-review it one more time to make sure, but I'm quite sure that the implementation works and is properly tested.

@ZeroIntensity ZeroIntensity self-requested a review December 1, 2024 19:50
This clarifies that we are referring to the email protocol, not the editor with the same name.

Co-authored-by: Guido van Rossum <[email protected]>
@foresto
Copy link
Author

foresto commented Dec 1, 2024

Suggestions committed, @gvanrossum. Thanks for the kind words, and for your help in getting this done.

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation mostly LGTM. I have a few docs nitpicks.

Doc/library/imaplib.rst Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Lib/imaplib.py Outdated Show resolved Hide resolved
@foresto
Copy link
Author

foresto commented Dec 2, 2024

I don't know why CI is suddenly failing since Guido merged main into this branch. The imaplib module hasn't changed in seven months, and the CI error output is useless:

empty-ci-error

Lib/imaplib.py Outdated Show resolved Hide resolved
This reverts and rephrases part of a3f21cd
which created links to a method on a deliberately undocumented class.
The links didn't work consistently, and caused sphinx warnings that
broke cpython's continuous integration tests.
This fixes a test that was broken by changing an exception in
b01de95
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a somewhat more thorough review of imaplib.py. I trust that @ZeroIntensity has reviewed the tests.

Lib/imaplib.py Outdated Show resolved Hide resolved
Lib/imaplib.py Outdated
responses via iteration, and sends DONE upon exit.
It represents responses as (type, datum) tuples, rather than the
(type, [data, ...]) tuples returned by other methods, because only one
response is represented at a time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
response is represented at a time.
response is read at a time.

(or "present"?)

Copy link
Author

@foresto foresto Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "delivered"?

I think I would prefer that over "read", since the latter could give the impression that we read only one response at a time from the server connection, which is not always true.

Lib/imaplib.py Show resolved Hide resolved
Note: The Idler class name and structure are internal interfaces,
subject to change. Calling code can rely on its context management,
iteration, and public method to remain stable, but should not
subclass, instantiate, or otherwise directly reference the class.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would checking isinstance(x, imaplib.Idler) be acceptable use? In that case you should probably commit to keeping the class name stable.

Copy link
Author

@foresto foresto Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a reason to perform such a check, so I would say no. I hoped that "or otherwise directly reference the class" would be sufficient discouragement, but I could make it explicit.

Of course, explicitly listing every possible discouraged use would get wordy pretty fast, thereby cluttering the docs, and maybe even giving readers an impression that using this interface is more complicated than it actually is.

I originally avoided both problems by simply not documenting the context object's name at all, but I was encouraged to do so (and to remove its underscore prefix) in order to document burst() with an explicit class and to make the doc string easy to find.

Bigger picture:

I've been trying to keep the context/iterator object from entering "documented" status, which would set in stone its structure, name, and location, because:

  • It is a new interface with substantially new semantics, and although I think it's designed well, I also understand that real-world use of anything new sometimes reveals ways in which it could be improved. Allowing as much as possible for future refinements seems wise here. If it turns out not to need refinements after it has been public for while, we can always just update the docs in a future release, naming the object and removing the restrictions.
  • While learning imaplib's internals so that I could properly extend it, I discovered one or two nontrivial flaws in its design that I believe could be fixed, if we don't paint ourselves into a corner by setting any more of its interfaces in stone than we already have. (This is not just a theoretical concern. Retaining backward compatibility was a bit of a challenge when designing this extension; it would have been significantly harder and more complex if certain imaplib interfaces had been documented.)
  • The discussions and other IMAP implementations that led me to write this code included ideas for new features that could potentially be implemented here some day. Keeping this code open to restructuring could make the difference between such features being possible or not.

Copy link
Author

@foresto foresto Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose another way to allow for breaking changes in the future would be to declare this entire interface provisional. We could then replace this note about specific allowed/forbidden uses of an ephemeral object with a shorter note applying to idle() and everything associated with it. The docs would be a little simpler.

The drawback, of course, would be making people afraid to build with it until it was no longer provisional. So it would be less useful until some unknown release in the future. That seems heavy-handed to me. People have been asking for IMAP IDLE for well over a decade, and it would be nice to tell them that it's finally supported.

Iteration produces (type, datum) tuples. They slightly differ
from the tuples returned by IMAP4.response(): The second item in the
tuple is a single datum, rather than a list of them, because only one
untagged response is produced at a time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, wouldn't it be easier for some user code if the format were the same (just always returning a list of one element)? The fact that you have to warn for this suggests there's something fishy here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t remember if the IMAP protocol allows literal strings in an IDLE response. But if that’s the case, I think you’d return a single such response over multiple iterations.

Copy link
Author

@foresto foresto Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vadmium wrote:

I don’t remember if the IMAP protocol allows literal strings in an IDLE response.

According to my reading of the RFCs, literal strings* are allowed in some untagged responses, like certain forms of the FETCH response. They are represented here in exactly the same way they are represented by imaplib's existing methods. In fact, it's the same old code that produces them in both cases.

*For anyone reading along, IMAP literal strings would look like this using C escape sequences:

{5}\r\nhello

These are distinct from IMAP quoted strings, which look like this:

"hello"

Copy link
Author

@foresto foresto Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gvanrossum wrote:

wouldn't it be easier for some user code if the format were the same (just always returning a list of one element)?

That's what an early draft of this code did. As I used it in my own application, I found that:

  • Having to unpack the item from a list was an awkward extra step that I came to resent after having to do it a few times.
  • Delivering the item in a list created an expectation at the call site that the list might include multiple items, and that callers should account for that. I was unhappy with encouraging users to complicate their code for something that cannot happen, and with forcing them to think about it in the first place.
  • Unpacking a single item from a list cluttered the client code, making it slightly harder to read.

When I simplified it by removing the list, I did consider the impact on documentation: Either having to explicitly describe each member of the tuple here (which would mostly duplicate what is already described the the IMAP4 class docs) or referencing the existing docs along with a special case as I did here (which is not ideal, as you pointed out).

I also considered the impact on working with imaplib as a whole, by deviating from the return type of existing methods. This didn't seem like a big deal at the time, since the calling convention here (context manager + iteration) is already quite different from method calls.

I ended up keeping it this way because I felt a special case in the docs was easy enough to understand, and once understood, it didn't continue to complicate an application maintainer's work.

With all that said, I don't feel married to doing it this way. If you prefer the old way, I can change it back, and live with the extra list unpacking in my own program. It would be more consistent, at least. I suppose it might also favor programs that use both response() and idle(), if they want to pass the tuples from both into a single processing function.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with IMAP literal strings is that imaplib divides one response into one or more tuples, terminated by a byte string. Essentially one list item per line plus literal string. Real example of a FETCH response from #68215 (comment):

>>> M.fetch('1', '(BODY.PEEK[HEADER.FIELDS (DATE)])')
  56:52.30 > b'NMFB14 FETCH 1 (BODY.PEEK[HEADER.FIELDS (DATE)])'
  56:52.68 < b'* 1 FETCH (BODY[HEADER.FIELDS (DATE)] {41}'
  56:52.68 read literal size 41
  56:52.68 < b')'
  56:52.68 < b'NMFB14 OK Success'
('OK',
 [(b'1 (BODY[HEADER.FIELDS (DATE)] {41}',
   b'Date: Fri, 17 Sep 2004 11:56:09 +1000\r\n\r\n'),
  b')'])

See how the one response with embedded literal has been split into data=[(prefix, literal), suffix]. I suspect if a similar IMAP literal was received before or during an IDLE command, your code would yield (prefix, literal) in one iteration, and only yield suffix in the next iteration.


if __debug__ and imap.debug >= 4:
imap._mesg(f'idle _pop({timeout}) reading')
imap._get_response() # Reads line, calls _append_untagged()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So IIUC if a (malicious or broken) server ever sends a partial line and then just hangs forever, the client also just hangs forever, uninterrupted by the intended duration. (Not just here but anywhere _get_response() is called.)

Is this an acceptable risk? Outside IDLE mode we are also vulnerable to this, but here we have a timeout and a select() call (and a potentially long timeout) that makes me worry more about this.

In modern client code this is generally something we worry about (never trust the server); with old protocols it may be less of an issue (presumably one should only connect to trusted IMAP4 servers).

This is not necessarily something to address, I just want to understand that you are okay with the consequences.


start = time.monotonic()

yield from iter(functools.partial(self._pop, interval, None), None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rewrite using a while-loop and a plain yield? That would make the code more readable, and you wouldn't need the antiquated iter(x, SENTINEL) form.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, materializing a partial + importing functools is an overkill IMO.

Comment on lines +1575 to +1577
if self._duration is not None:
elapsed = time.monotonic() - start
self._duration = max(self._duration - elapsed, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a bit complicated. You subtract the elapsed time since start from the (remaining) duration, but this allows for small amounts of drift to creep in due to time unaccounted for.

One algorithm I've used is to calculate the (absolute) end time (as time.monotonic() + self._duration) once, when the context manager is entered, and then only call time.monotonic() just before calling select(), or actually, before calling _wait(), to compute the timeout to pass to select()`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with using the deadline technique. Looks like it could help:

  • account for time spent by the caller (before iteration, between yield points, etc), which looks inconsistently handled at the moment
  • count real time exactly once, even if a burst iterator is never exhausted, or concurrent burst iterators are used
  • reusing an Idler object to repeat an IDLE command with the original specified time limit

Copy link
Member

@vadmium vadmium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few general comments in addition my comments on the diff:

Method to read one response or time out

I wonder if providing a lower-level method, that either returns a single response or times out and say returns None, might cover more use cases? The two higher-level features (dur time limit and burst collecting) could be implemented in terms of it:

with idler:
    deadline = time.monotonic() + dur
    while response := idler.read(deadline - time.monotonic()):
        handle_response(response)

with idler:
    batch = [idler.read()]
    while response := idler.read(timeout_secs=0.1):
        batch.append(response)
    handle_burst(batch)

Timeout data types

There is an example specifying int for idle(dur=. . .), while burst(interval=. . .) defaults to float. It would be good to explicitly say what types are allowed for each parameter.

Lib/imaplib.py Outdated Show resolved Hide resolved
print(typ, datum)

('EXISTS', b'1')
('RECENT', b'1')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a mistake? Why is this apparently printing a single tuple each loop? It reminds me of trying to call the Python 2 print statement.

Might also be worth using the >>> prompt notation like done under the IMAP4 constructor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to use a pycon code-block together with the >>> notation. Here, the exemple would render as a single code block and ('EXIST', b'1') would be a statement (that does nothing).

print(typ, datum)
Responses produced by the iterator are not added to the internal
cache for retrieval by response().
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems an important point that should be included in the main RST documentation. And what about responses yielded from burst?

the next response along with any immediately available subsequent responses
(e.g. a rapid series of ``EXPUNGE`` events from a bulk delete). This
batch processing aid is provided by the context's ``burst()``
:term:`generator`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be better to move this paragraph under the Idler.burst heading? I missed the clue that burst is a generator the first few passes through trying to understand the changes.


The context manager sends the ``IDLE`` command when activated by the
:keyword:`with` statement, produces IMAP untagged responses via the
:term:`iterator` protocol, and sends ``DONE`` upon context exit.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth clarifying that the Idler iterable should only be iterated inside its with statements, i.e. you have to enter with to get unsolicited responses sent before the idle command started, and you have to use the general response method after exit

Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Comment on lines +1575 to +1577
if self._duration is not None:
elapsed = time.monotonic() - start
self._duration = max(self._duration - elapsed, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with using the deadline technique. Looks like it could help:

  • account for time spent by the caller (before iteration, between yield points, etc), which looks inconsistently handled at the moment
  • count real time exactly once, even if a burst iterator is never exhausted, or concurrent burst iterators are used
  • reusing an Idler object to repeat an IDLE command with the original specified time limit

Iteration produces (type, datum) tuples. They slightly differ
from the tuples returned by IMAP4.response(): The second item in the
tuple is a single datum, rather than a list of them, because only one
untagged response is produced at a time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t remember if the IMAP protocol allows literal strings in an IDLE response. But if that’s the case, I think you’d return a single such response over multiple iterations.


with selectors.DefaultSelector() as sel:
sel.register(fileobj, selectors.EVENT_READ)
readables = sel.select(timeout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect using selectors on an SSL socket as if it is a low-level OS socket may not be reliable; see https://docs.python.org/dev/library/ssl.html#notes-on-non-blocking-sockets. Maybe you can get away with setting a timeout before reading the SSL socket instead?

@vadmium
Copy link
Member

vadmium commented Dec 2, 2024

Looks like the IMAP4.file attribute is no longer really used. I wonder if it should it be removed. Any external code using it would probably already be broken by the separate _readbuf buffering anyway.

This makes it more obvious which statement triggers the branch.
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments. I can review more in details in a few days (I think we still need Serhiy's review).

Doc/library/imaplib.rst Outdated Show resolved Hide resolved
The *duration* argument sets a maximum duration (in seconds) to keep idling,
after which any ongoing iteration will stop. It defaults to ``None``,
meaning no time limit. Callers wishing to avoid inactivity timeouts on
servers that impose them should keep this at most 29 minutes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also mention that 29 minutes is 1740 seconds since the duration's unit is the second.

Suggested change
servers that impose them should keep this at most 29 minutes.
servers that impose them should keep this at most 29 minutes (1740 seconds).

See the :ref:`warning below <windows-pipe-timeout-warning>` if using
:class:`IMAP4_stream` on Windows.

Response tuples produced by the iterator almost exactly match those
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a native English speaker but I would simply say "almost match" because having "almost exactly" rings contradictory.

print(typ, datum)

('EXISTS', b'1')
('RECENT', b'1')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to use a pycon code-block together with the >>> notation. Here, the exemple would render as a single code block and ('EXIST', b'1') would be a statement (that does nothing).


Instead of iterating one response at a time, it is also possible to retrieve
the next response along with any immediately available subsequent responses
(e.g. a rapid series of ``EXPUNGE`` events from a bulk delete). This
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(e.g. a rapid series of ``EXPUNGE`` events from a bulk delete). This
(e.g., a rapid series of ``EXPUNGE`` events from a bulk delete). This

Comment on lines +1471 to +1474
if timeout is None:
return True
if imap._readbuf:
return True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if timeout is None:
return True
if imap._readbuf:
return True
if timeout is None or imap._readbuf:
return True


if imap.sock:
fileobj = imap.sock
elif platform.system() == 'Windows':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use os.name == 'nt' instead? that's how we generally check Windows-related code.

if __debug__ and imap.debug >= 4:
imap._mesg(f'idle _wait select({timeout})')

with selectors.DefaultSelector() as sel:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reduce import time, you could locally import selectors here.


start = time.monotonic()

yield from iter(functools.partial(self._pop, interval, None), None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, materializing a partial + importing functools is an overkill IMO.

with client.idle():
pass

class IdleCmdHandler(SimpleIMAPHandler):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a non-local class? other helper classes are also non-local

foresto and others added 3 commits December 2, 2024 23:39
The burst() method is a little tricky to link in restructuredText, due
to quirks of its parent class.  This syntax allows sphinx to generate
working links without generating warnings (which break continuous
integration) and without burdening the reader with unimportant namespace
qualifications.  It makes the reST source ugly, but few people read
the reST source, so it's a tolerable tradeoff.
@serhiy-storchaka
Copy link
Member

Sorry, I've had other things to do during these two months and just forgot about this PR. I'll try to get the review done by the end of the week, but I don't have access to my computer right now. Please don't hesitate to ping me if I'm late.

@python python deleted a comment from gvanrossum-ms Dec 4, 2024
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let's wait for Serhiy. (Feel free to ping him at the time he said.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants