[Firstaidkit-devel] Re: [PATCHes] Add question support

[Martin Sivak]

Hi,

I have some more "flaws" to discuss:

+ ANSWER = 999#In question reply, {"question", "answer"}

This says nothing about the type of the answer, the response should use common info, tree or table type to create the answer and use the destination argument in the put call to select the answer queue. Or use the proper method of the newly created mailbox to construct it. (see the comments about openMailbox below)

Also you should use the standard message structure for passing the information around, so answer goes to "message" and at least the origin field is set.

+    @staticmethod
+    def __blocking_question(fn, args, kwargs):
+        queue = Queue.Queue()
+        question = fn(queue, *args, **kwargs)
+        r = queue.get()
+        assert r["action"] == ANSWER and r["question"] is question
+        return r["answer"]

You should use openMailbox and closeMailbox for queue management. The assert shouldn't be checking for ANSWER (see above). And I'd rename the "question" field to "inreplyto".

+    def __init__(self, queue, importance = logging.INFO, secondary = False

Better name for the secondary argument would be "interactive" with default value of True

I added the inreplyto field and modified the openMailbox/closeMailbox in such a way, that you can call the standard methods (info, table, tree, ..) of the returned mailbox. So you shouldn't need to construct the message structure by yourself.

I also applied some of your patches.

Martin

----- "Miloslav Trmac" <mitr at redhat.com> wrote:

> Hello,
> ----- "Miloslav Trmac" <mitr at redhat.com> wrote:
> > the attached patches add support for asking the user questions, and
> an
> > example plugin to demonstrate the functionality.
> Attached is an updated "main" patch, with the following changes:
> * Use a queue for delivering replies, to eventually support
> event-driven plugins
> * Use a separate message["action"] type for each question class
> * Store the Question object in message["message"] instead of
> message["question"],
>   store prompt inside the Question object.
> Thanks to Martin Sivak for a review of the original patch.
>     Mirek
> _______________________________________________
> Firstaidkit-devel mailing list
> Firstaidkit-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/firstaidkit-devel