unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* python: unpythonic result of Message.get_replies()
@ 2011-10-05  1:42 Justus Winter
  2011-11-02  7:29 ` Sebastian Spaeth
  0 siblings, 1 reply; 9+ messages in thread
From: Justus Winter @ 2011-10-05  1:42 UTC (permalink / raw)
  To: notmuch

[-- 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 @,@


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: python: unpythonic result of Message.get_replies()
  2011-10-05  1:42 python: unpythonic result of Message.get_replies() Justus Winter
@ 2011-11-02  7:29 ` Sebastian Spaeth
  2011-12-21 13:15   ` Justus Winter
  0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Spaeth @ 2011-11-02  7:29 UTC (permalink / raw)
  To: Justus Winter, notmuch

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: python: unpythonic result of Message.get_replies()
  2011-11-02  7:29 ` Sebastian Spaeth
@ 2011-12-21 13:15   ` Justus Winter
  2011-12-21 13:15     ` [PATCH 1/2] python: refactor print_messages into format_messages and print_messages Justus Winter
  2011-12-21 13:15     ` [PATCH 2/2] python: make the result of Message.get_replies() more pythonic Justus Winter
  0 siblings, 2 replies; 9+ messages in thread
From: Justus Winter @ 2011-12-21 13:15 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] python: refactor print_messages into format_messages and print_messages
  2011-12-21 13:15   ` Justus Winter
@ 2011-12-21 13:15     ` Justus Winter
  2012-01-02 13:13       ` Tomi Ollila
  2012-01-02 15:56       ` Sebastian Spaeth
  2011-12-21 13:15     ` [PATCH 2/2] python: make the result of Message.get_replies() more pythonic Justus Winter
  1 sibling, 2 replies; 9+ messages in thread
From: Justus Winter @ 2011-12-21 13:15 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] python: make the result of Message.get_replies() more pythonic
  2011-12-21 13:15   ` Justus Winter
  2011-12-21 13:15     ` [PATCH 1/2] python: refactor print_messages into format_messages and print_messages Justus Winter
@ 2011-12-21 13:15     ` Justus Winter
  2012-01-02 13:15       ` Tomi Ollila
  2012-01-02 16:03       ` Sebastian Spaeth
  1 sibling, 2 replies; 9+ messages in thread
From: Justus Winter @ 2011-12-21 13:15 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] python: refactor print_messages into format_messages and print_messages
  2011-12-21 13:15     ` [PATCH 1/2] python: refactor print_messages into format_messages and print_messages Justus Winter
@ 2012-01-02 13:13       ` Tomi Ollila
  2012-01-02 15:56       ` Sebastian Spaeth
  1 sibling, 0 replies; 9+ messages in thread
From: Tomi Ollila @ 2012-01-02 13:13 UTC (permalink / raw)
  To: Justus Winter, notmuch

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] python: make the result of Message.get_replies() more pythonic
  2011-12-21 13:15     ` [PATCH 2/2] python: make the result of Message.get_replies() more pythonic Justus Winter
@ 2012-01-02 13:15       ` Tomi Ollila
  2012-01-02 16:03       ` Sebastian Spaeth
  1 sibling, 0 replies; 9+ messages in thread
From: Tomi Ollila @ 2012-01-02 13:15 UTC (permalink / raw)
  To: Justus Winter, notmuch

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] python: refactor print_messages into format_messages and print_messages
  2011-12-21 13:15     ` [PATCH 1/2] python: refactor print_messages into format_messages and print_messages Justus Winter
  2012-01-02 13:13       ` Tomi Ollila
@ 2012-01-02 15:56       ` Sebastian Spaeth
  1 sibling, 0 replies; 9+ messages in thread
From: Sebastian Spaeth @ 2012-01-02 15:56 UTC (permalink / raw)
  To: Justus Winter, notmuch

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] python: make the result of Message.get_replies() more pythonic
  2011-12-21 13:15     ` [PATCH 2/2] python: make the result of Message.get_replies() more pythonic Justus Winter
  2012-01-02 13:15       ` Tomi Ollila
@ 2012-01-02 16:03       ` Sebastian Spaeth
  1 sibling, 0 replies; 9+ messages in thread
From: Sebastian Spaeth @ 2012-01-02 16:03 UTC (permalink / raw)
  To: Justus Winter, notmuch

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2012-01-02 16:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-05  1:42 python: unpythonic result of Message.get_replies() Justus Winter
2011-11-02  7:29 ` Sebastian Spaeth
2011-12-21 13:15   ` Justus Winter
2011-12-21 13:15     ` [PATCH 1/2] python: refactor print_messages into format_messages and print_messages Justus Winter
2012-01-02 13:13       ` Tomi Ollila
2012-01-02 15:56       ` Sebastian Spaeth
2011-12-21 13:15     ` [PATCH 2/2] python: make the result of Message.get_replies() more pythonic Justus Winter
2012-01-02 13:15       ` Tomi Ollila
2012-01-02 16:03       ` Sebastian Spaeth

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).