python: unpythonic result of Message.get_replies()

python: unpythonic result of Message.get_replies()

[Justus Winter]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]


Hi everyone :)

I noticed that Message.get_replies() returns a Messages object *or*
None. Quoting the documentation:

> Returns: Messages or None if there are no replies to this message

Messages is a class implementing the iterator protocol, so a python
programmer would expect to get an iterator that raises a StopIteration
on the first invocation of next() if there aren't any replies.

With the current implementation one needs to do something like

replies = message.get_replies()
if replies != None:
    for reply in replies:
        [...]

which looks awkward. Imho one should be able to just do

for reply in message.get_replies():
    [...]

What do you think? Would it be possible to get this into the 0.9
release? I could propose a patch if you like, but not today...

Justus

[-- Attachment #2: .signature --]
[-- Type: application/octet-stream, Size: 17 bytes --]

love u alot @,@

Re: python: unpythonic result of Message.get_replies()

[Sebastian Spaeth]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

On Wed, 05 Oct 2011 03:42:38 +0200, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
Non-text part: multipart/mixed
> I noticed that Message.get_replies() returns a Messages object *or*
> None. Quoting the documentation:
> 
> > Returns: Messages or None if there are no replies to this message
> 
> Messages is a class implementing the iterator protocol, so a python
> programmer would expect to get an iterator that raises a StopIteration
> on the first invocation of next() if there aren't any replies.
> 

Yes, that change would make perfect sense, and I would be happy to
accept a patch for it :-)

Sebastian

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: python: unpythonic result of Message.get_replies()

[Justus Winter]

The attached patch series fixes this problem. Note that the wrapping
nature of the notmuch bindings makes it kind of awkward to fix the
behavior.

I've decided to avoid introducing code to the Messages class to
indicate that there are no messages and there is no notmuch object
being wrapped, but to subclass it and change the constructor and
__next__ function.

Well, what do you think?
Justus

[PATCH 1/2] python: refactor print_messages into format_messages and print_messages

[Justus Winter]

---
 bindings/python/notmuch/message.py |   37 +++++++++++++++++++++++++----------
 1 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/bindings/python/notmuch/message.py b/bindings/python/notmuch/message.py
index ce8e718..cc9fc2a 100644
--- a/bindings/python/notmuch/message.py
+++ b/bindings/python/notmuch/message.py
@@ -186,14 +186,17 @@ class Messages(object):
         if self._msgs is not None:
             self._destroy(self._msgs)
 
-    def print_messages(self, format, indent=0, entire_thread=False):
-        """Outputs messages as needed for 'notmuch show' to sys.stdout
+    def format_messages(self, format, indent=0, entire_thread=False):
+        """Formats messages as needed for 'notmuch show'.
 
         :param format: A string of either 'text' or 'json'.
         :param indent: A number indicating the reply depth of these messages.
         :param entire_thread: A bool, indicating whether we want to output
                        whole threads or only the matching messages.
+        :return: a list of lines
         """
+        result = list()
+
         if format.lower() == "text":
             set_start = ""
             set_end = ""
@@ -207,36 +210,48 @@ class Messages(object):
 
         first_set = True
 
-        sys.stdout.write(set_start)
+        result.append(set_start)
 
         # iterate through all toplevel messages in this thread
         for msg in self:
             # if not msg:
             #     break
             if not first_set:
-                sys.stdout.write(set_sep)
+                result.append(set_sep)
             first_set = False
 
-            sys.stdout.write(set_start)
+            result.append(set_start)
             match = msg.is_match()
             next_indent = indent
 
             if (match or entire_thread):
                 if format.lower() == "text":
-                    sys.stdout.write(msg.format_message_as_text(indent))
+                    result.append(msg.format_message_as_text(indent))
                 else:
-                    sys.stdout.write(msg.format_message_as_json(indent))
+                    result.append(msg.format_message_as_json(indent))
                 next_indent = indent + 1
 
             # get replies and print them also out (if there are any)
             replies = msg.get_replies()
             if not replies is None:
-                sys.stdout.write(set_sep)
-                replies.print_messages(format, next_indent, entire_thread)
+                result.append(set_sep)
+                result.extend(replies.format_messages(format, next_indent, entire_thread))
+
+            result.append(set_end)
+        result.append(set_end)
 
-            sys.stdout.write(set_end)
-        sys.stdout.write(set_end)
+        return result
 
+    def print_messages(self, format, indent=0, entire_thread=False, handle=sys.stdout):
+        """Outputs messages as needed for 'notmuch show' to a file like object.
+
+        :param format: A string of either 'text' or 'json'.
+        :param handle: A file like object to print to (default is sys.stdout).
+        :param indent: A number indicating the reply depth of these messages.
+        :param entire_thread: A bool, indicating whether we want to output
+                       whole threads or only the matching messages.
+        """
+        handle.write(''.join(self.format_messages(format, indent, entire_thread)))
 
 class Message(object):
     """Represents a single Email message
-- 
1.7.7.3

[PATCH 2/2] python: make the result of Message.get_replies() more pythonic

[Justus Winter]

Formerly Message.get_replies() returned an iterator or None forcing
users to check the result before iterating over it leading to strange
looking code at the call site.

Fix this flaw by adding an EmptyMessagesResult class that behaves like
the Messages class but immediatly raises StopIteration if used as an
iterator and returning objects of this type from Message.get_replies()
to indicate that there are no replies.
---
 bindings/python/notmuch/message.py |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/bindings/python/notmuch/message.py b/bindings/python/notmuch/message.py
index cc9fc2a..975db1c 100644
--- a/bindings/python/notmuch/message.py
+++ b/bindings/python/notmuch/message.py
@@ -232,10 +232,10 @@ class Messages(object):
                 next_indent = indent + 1
 
             # get replies and print them also out (if there are any)
-            replies = msg.get_replies()
-            if not replies is None:
+            replies = msg.get_replies().format_messages(format, next_indent, entire_thread)
+            if replies:
                 result.append(set_sep)
-                result.extend(replies.format_messages(format, next_indent, entire_thread))
+                result.extend(replies)
 
             result.append(set_end)
         result.append(set_end)
@@ -253,6 +253,15 @@ class Messages(object):
         """
         handle.write(''.join(self.format_messages(format, indent, entire_thread)))
 
+class EmptyMessagesResult(Messages):
+    def __init__(self, parent):
+        self._msgs = None
+        self._parent = parent
+
+    def __next__(self):
+        raise StopIteration()
+    next = __next__
+
 class Message(object):
     """Represents a single Email message
 
@@ -383,10 +392,9 @@ class Message(object):
             number of subsequent calls to :meth:`get_replies`). If this message
             was obtained through some non-thread means, (such as by a call to
             :meth:`Query.search_messages`), then this function will return
-            `None`.
+            an empty Messages iterator.
 
-        :returns: :class:`Messages` or `None` if there are no replies to
-            this message.
+        :returns: :class:`Messages`.
         :exception: :exc:`NotmuchError` STATUS.NOT_INITIALIZED if the message
                     is not initialized.
         """
@@ -396,7 +404,7 @@ class Message(object):
         msgs_p = Message._get_replies(self._msg)
 
         if msgs_p is None:
-            return None
+            return EmptyMessagesResult(self)
 
         return Messages(msgs_p, self)
 
-- 
1.7.7.3

Re: [PATCH 1/2] python: refactor print_messages into format_messages and print_messages

[Tomi Ollila]

On Wed, 21 Dec 2011 14:15:01 +0100, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> ---
>  bindings/python/notmuch/message.py |   37 +++++++++++++++++++++++++----------
>  1 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/bindings/python/notmuch/message.py b/bindings/python/notmuch/message.py
> index ce8e718..cc9fc2a 100644
> --- a/bindings/python/notmuch/message.py
> +++ b/bindings/python/notmuch/message.py

LGTM.

Re: [PATCH 2/2] python: make the result of Message.get_replies() more pythonic

[Tomi Ollila]

On Wed, 21 Dec 2011 14:15:02 +0100, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> Formerly Message.get_replies() returned an iterator or None forcing
> users to check the result before iterating over it leading to strange
> looking code at the call site.
> 
> Fix this flaw by adding an EmptyMessagesResult class that behaves like
> the Messages class but immediatly raises StopIteration if used as an
> iterator and returning objects of this type from Message.get_replies()
> to indicate that there are no replies.
> ---
>  bindings/python/notmuch/message.py |   22 +++++++++++++++-------
>  1 files changed, 15 insertions(+), 7 deletions(-)

LGTM, but does this break any current software using this API ?

Tomi

Re: [PATCH 1/2] python: refactor print_messages into format_messages and print_messages

[Sebastian Spaeth]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]

On Wed, 21 Dec 2011 14:15:01 +0100, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> ---
>  bindings/python/notmuch/message.py |   37 +++++++++++++++++++++++++----------
>  1 files changed, 26 insertions(+), 11 deletions(-)

Pushed

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/2] python: make the result of Message.get_replies() more pythonic

[Sebastian Spaeth]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

On Wed, 21 Dec 2011 14:15:02 +0100, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> Formerly Message.get_replies() returned an iterator or None forcing
> users to check the result before iterating over it leading to strange
> looking code at the call site.
> 
> Fix this flaw by adding an EmptyMessagesResult class that behaves like
> the Messages class but immediatly raises StopIteration if used as an
> iterator and returning objects of this type from Message.get_replies()
> to indicate that there are no replies.


Makes sense, pushed. It shouldn't cause the breaking of existing
clients... (famous last words)

Sebastian

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]