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

["Niteesh G. S." <niteesh.gs@gmail.com>]

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

On Wed, Aug 18, 2021 at 11:52 PM John Snow <jsnow@redhat.com> wrote:

>
>
> 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 :)
>
Maybe we'll come back to this on v4 :)

>
>
>> +
>>>> +
>>>> +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?
>
IMHO Yes it looks pretty.

[1, true, 3]: QMP message is not a JSON object.
This one looks much better than the below one
[ 1,
       true,
3 ]: QMP message is not a JSON object.


>
>> +
>>>> +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' + ...
>
Thanks. Fixed.

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