Re: [PATCH v3 04/13] python/aqmp-tui: Add AQMP TUI draft

From: John Snow <jsnow@redhat.com>
To: "Niteesh G. S." <niteesh.gs@gmail.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	Kashyap Chamarthy <kchamart@redhat.com>,
	Markus Armbruster <armbru@redhat.com>,
	Wainer Moschetta <wainersm@redhat.com>,
	qemu-devel <qemu-devel@nongnu.org>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Cleber Rosa <crosa@redhat.com>, Eric Blake <eblake@redhat.com>
Subject: Re: [PATCH v3 04/13] python/aqmp-tui: Add AQMP TUI draft
Date: Wed, 18 Aug 2021 14:22:20 -0400	[thread overview]
Message-ID: <CAFn=p-byqejuz=4_dZWTEw_ab4fq_ii--XPdLGhTQjwLPsrFGw@mail.gmail.com> (raw)
In-Reply-To: <CAN6ztm9r9VACvnUJ72anDmpy7hsQi=tis5Lkd-vC8NE0DCFAPg@mail.gmail.com>

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

On Fri, Aug 13, 2021 at 11:11 AM Niteesh G. S. <niteesh.gs@gmail.com> wrote:

>
> On Fri, Aug 6, 2021 at 12:28 AM John Snow <jsnow@redhat.com> wrote:
>
>>
>> On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
>> wrote:
>>
>>> Added a draft of AQMP TUI.
>>>
>>> Implements the follwing basic features:
>>> 1) Command transmission/reception.
>>> 2) Shows events asynchronously.
>>> 3) Shows server status in the bottom status bar.
>>>
>>> Also added necessary pylint, mypy configurations
>>>
>>> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
>>>
>>
...

> +
>>> +# Using root logger to enable all loggers under qemu and asyncio
>>> +LOGGER = logging.getLogger()
>>>
>>
>> The comment isn't quite true; this is the root logger -- but you go on to
>> use it to directly log messages. I don't think you should; use a
>> module-level logger.
>>
>> (1) Create a module-level logger that is named after the current module
>> name (e.g. qemu.aqmp.aqmp_tui) and use that for logging messages relating
>> to this module:
>> LOGGER = logging.getLogger(__name__)
>>
>> (2) Where you need to address the root logger, just use `root_logger =
>> logging.getLogger() ` .... I think the main() function is the only place
>> you might need this.
>>
> The way of logging in the TUI is changed such that, all logging is done
> through the root level logger. The handlers and levels are installed to
> the root level
> logger to allow logging from other libraries to be rerouted to the screen
> or file.
>

We talked about this on IRC, so I'll assume our disagreement in vision here
is cleared up ... or at least different :)

> +
>>> +
>>> +def format_json(msg):
>>> +    """
>>> +    Formats given multiline JSON message into a single line message.
>>> +    Converting into single line is more asthetically pleasing when
>>> looking
>>> +    along with error messages compared to multiline JSON.
>>> +    """
>>> +    # FIXME: Use better formatting mechanism. Might break at more
>>> complex JSON
>>> +    # data.
>>> +    msg = msg.replace('\n', '')
>>> +    words = msg.split(' ')
>>> +    words = [word for word in words if word != '']
>>> +    return ' '.join(words)
>>> +
>>>
>>
>> You can use the JSON module to do this,
>> https://docs.python.org/3/library/json.html#json.dumps
>>
>> Message._serialize uses this technique to send JSON messages over the
>> wire that have no newlines:
>>
>> https://gitlab.com/jsnow/qemu/-/blob/python-async-qmp-aqmp/python/qemu/aqmp/message.py#L132
>>
>> by not specifying an indent and including separators with no spaces, we
>> can get a JSON representation with all the space squeezed out. You can add
>> spaces and still omit the indent/newlines and so on.
>>
>

> But will this work for invalid JSON messages? As far as I have tried it
> doesn't work.
>

Ah, I see ... Nope, that requires a valid JSON message ... Do we *need* to
pretty-print invalid JSON?

> +
>>> +def main():
>>> +    parser = argparse.ArgumentParser(description='AQMP TUI')
>>> +    parser.add_argument('qmp_server', help='Address of the QMP server'
>>> +                        '< UNIX socket path | TCP addr:port >')
>>> +    parser.add_argument('--log-file', help='The Log file name')
>>> +    parser.add_argument('--log-level', default='WARNING',
>>> +                        help='Log level
>>> <CRITICAL|ERROR|WARNING|INFO|DEBUG|>')
>>> +    parser.add_argument('--asyncio-debug', action='store_true',
>>> +                        help='Enable debug mode for asyncio loop'
>>> +                        'Generates lot of output, makes TUI unusable
>>> when'
>>> +                        'logs are logged in the TUI itself.'
>>> +                        'Use only when logging to a file')
>>>
>>
>> Needs spaces between the lines here so that the output reads correctly.
>>
> The output renders properly for me. Can you please post a pic if it
> doesn't render correctly for you?
>

No screenshot needed, just try running with '--help'. When you concatenate
strings like this:

parser.add_argument('--asyncio-debug', action='store_true',
                    help='Enable debug mode for asyncio loop'
                    'Generates lot of output, makes TUI unusable when'
                    'logs are logged in the TUI itself.'
                    'Use only when logging to a file')
Python is going to do this:

help='Enable debug mode for asyncio loop' + 'Generates lot of output, makes
TUI unusable when' + ...

so you'll get a string like this:

help='Enable debug mode for asyncio loopGenerates lot of output, makes TUI
unusable when' + ...

[-- Attachment #2: Type: text/html, Size: 8613 bytes --]