Re: [PATCH v4 06/14] qapi/introspect.py: replace 'extra' dict with 'comment' argument

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 2/3/21 9:23 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> This is only used to pass in a dictionary with a comment already set, so
>>> skip the runaround and just accept the comment.
>>>
>>> This works because _tree_to_qlit() treats 'if': None; 'comment': None
>>> exactly like absent 'if'; 'comment'.
>> Confusing, because the two paragraphs talk about two different
>> things:
>> 1. Actual arguments for @extra are either None or {'comment':
>> comment}.
>>     Simplify: replace parameter @extra by parameter @comment.
>> 2. Dumb down the return value to always be of the form
>>      (obj {'if': ifcond, 'comment': comment})
>> 
>
> I think you are drawing attention to the fact that 'if' and 'comment'
> are now always present in this dict instead of conditionally present.

Correct.

> (else, I have misread you. (I think you are missing a comma.))

I am!  I meant to write

    (obj, {'if': ifcond, 'comment': comment})

>> I suspect splitting the patch is easier than crafting a clear commit
>> message for the combined one.
>> 
>
> I wouldn't have considered to break out such a small change into two
> even smaller changes, but as you are in charge here ... Okey Dokey.
>
> (meta-tangent: [1])
[...]
> [1] As a matter of process, I sometimes find it cumbersome to
> intentionally engineer an intermediary state when I jumped straight
> from A->C in my actual editing.

Yes, the extra work can be cumbersome.  But then writing a neat commit
message for a commit that does two things can also be cumbersome.
"Split and write two straightforward commit messages" has proven easier
for me many times.

> I will usually keep such intermediary forms when they come about
> naturally in the course of development, but rarely seek to add them 
> artificially -- it feels like a major bummer to engineer, test, and
> scrutinize code that's only bound to be deleted immediately after. 
> Sometimes, it feels like a waste of reviewer effort, too.

It depends.  Sometimes "don't split and write a complicated commit
message" is easier.

Which way you get to "commit message(s) don't confuse Markus" in this
particular case is up to you :)

> It's been years and I still don't think I have any real intuitive
> sense for this, which is ...unfortunate.

It's been years, and my intuition still evolves.