python: unpythonic result of Message.get_replies()

classic Classic list List threaded Threaded
9 messages Options
Justus Winter Justus Winter
Reply | Threaded
Open this post in threaded view
|

python: unpythonic result of Message.get_replies()


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

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

.signature (26 bytes) Download Attachment
spaetz spaetz
Reply | Threaded
Open this post in threaded view
|

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

On Wed, 05 Oct 2011 03:42:38 +0200, Justus Winter <[hidden email]> 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

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

attachment0 (203 bytes) Download Attachment
Justus Winter Justus Winter
Reply | Threaded
Open this post in threaded view
|

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

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
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Justus Winter Justus Winter
Reply | Threaded
Open this post in threaded view
|

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

---
 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

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Justus Winter Justus Winter
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by 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

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Justus Winter
On Wed, 21 Dec 2011 14:15:01 +0100, Justus Winter <[hidden email]> 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.
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Justus Winter
On Wed, 21 Dec 2011 14:15:02 +0100, Justus Winter <[hidden email]> 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

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
spaetz spaetz
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Justus Winter
On Wed, 21 Dec 2011 14:15:01 +0100, Justus Winter <[hidden email]> wrote:
> ---
>  bindings/python/notmuch/message.py |   37 +++++++++++++++++++++++++----------
>  1 files changed, 26 insertions(+), 11 deletions(-)

Pushed

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

attachment0 (203 bytes) Download Attachment
spaetz spaetz
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Justus Winter
On Wed, 21 Dec 2011 14:15:02 +0100, Justus Winter <[hidden email]> 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

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

attachment0 (203 bytes) Download Attachment