John Snow <js...@redhat.com> writes:
> On 5/18/21 5:28 AM, Markus Armbruster wrote:
>> John Snow <js...@redhat.com> writes:
>>
>>> Remove the try/except block that handles file-opening errors in
>>> QAPISchemaParser.__init__() and add one each to
>>> QAPISchemaParser._include() and QAPISchema.__init__() respectively.
>>>
>>>
>>> The short-ish version of what motivates this patch is:
>>>
>>> - It's hard to write a good error message in the init method,
>>> because we need to determine the context of our caller to do so.
>>> It's easier to just let the caller write the message.
>>
>> I kind of disagree, but that's okay; it's your commit message :)
>>
>
> I can nix the paragraph if you want, the primary purpose was to explain
> to you what I was thinking anyway, and you already know ;)
Nah, keep it.
>>> - We don't want to allow QAPISourceInfo(None, None, None) to exist.
>>> - Errors made using such an object are currently incorrect.
>>> - It's not technically a semantic error if we cannot open the schema
>>> - There are various typing constraints that make mixing these two cases
>>> undesirable for a single special case.
>>>
>>> Other considerations:
>>>
>>> - open() is moved to a 'with' block to ensure file pointers are
>>> cleaned up deterministically.
>>
>> Improvement over v1's leak claim. Sold!
>>
>>> - Python 3.3 deprecated IOError and made it a synonym for OSError.
>>> Avoid the misleading perception these exception handlers are
>>> narrower than they really are.
>>> - Not all QAPIError objects have an 'info' field, so remove that stanza
>>> from test-qapi. Don't bother to replace the early exit on purpose
>>> so that we can test its output in the next commit.
>>
>> To which hunk exactly does the last item refer?
>>
>
> Sigh, "Early return", not *exit* -- and I'm referring to the test-qapi hunk.
>
>> My best guess is the last one. Its rationale is actually "drop code
>> handling the variant of QAPISourceInfo being removed in this patch".
>>
>
> That too ... I just meant to say "It doesn't need to be replaced"
Can we the commit message clearer here? Maybe:
- test-qapi's code handling None fname is now dead. Drop it.
Or just drop the item entirely.
>> QAPIError not having .info don't actually exist before this patch.
>>
>> I'm afraid I don't get the second sentence.
>> >>
>>>
>>> The long version:
>>>
>>> The error message string here is incorrect (since 52a474180a):
>>
>> I think this reads slightly better:
>>
>> The error message here is incorrect (since commit 52a474180a):
>
> OK (If I need to respin I'll change it?)
Or I change it in my tree if we decide we don't need a full respin.
>>>> python3 qapi-gen.py 'fake.json'
>>> qapi-gen.py: qapi-gen.py: can't read schema file 'fake.json': No such file
>>> or directory
>>>
>>> In pursuing it, we find that QAPISourceInfo has a special accommodation
>>> for when there's no filename. Meanwhile, the intended typing of
>>> QAPISourceInfo was (non-optional) 'str'.
>>
>> Not sure about "intended". When I wrote the code, I intended ".fname is
>> str means it's a position that file; None means it's not in a file". I
>> didn't intend typing, because typing wasn't a concern back then.
>>
>> Do you mean something like "we'd prefer to type .fname as (non-optional)
>> str"?
>>
>
> Really, I meant *I* intended that typing. I just have a habit of using
> "we" for F/OSS commit messages.
>
> What I am really saying: When I typed this field, I didn't realize it
> could be None actually -- I see this as a way to fix the "intended
> typing" that I established however many commits ago.
In commit f5d4361cda "qapi/source.py: add type hint annotations", I
believe.
Hmm, this commit actually fixes incorrect typing, doesn't it?
>>>
>>> To remove this, I'd want to avoid having a "fake" QAPISourceInfo
>>> object. We also don't want to explicitly begin accommodating
>>> QAPISourceInfo itself being None, because we actually want to eventually
>>> prove that this can never happen -- We don't want to confuse "The file
>>> isn't open yet" with "This error stems from a definition that wasn't
>>> defined in any file".
>>>
>>> (An earlier series tried to create a dummy info object, but it was tough
>>> to prove in review that it worked correctly without creating new
>>> regressions. This patch avoids that distraction. We would like to first
>>> prove that we never raise QAPISemError for any built-in object before we
>>> add "special" info objects. We aren't ready to do that yet.)
>>>
>>> So, which way out of the labyrinth?
>>>
>>> Here's one way: Don't try to handle errors at a level with "mixed"
>>> semantic contexts; i.e. don't mix inclusion errors (should report a
>>> source line where the include was triggered) and command line errors
>>> (where we specified a file we couldn't read).
>>>
>>> Remove the error handling from the initializer of the parser. Pythonic!
>>> Now it's the caller's job to figure out what to do about it. Handle the
>>> error in QAPISchemaParser._include() instead, where we can write a
>>> targeted error message where we are guaranteed to have an 'info' context
>>> to report with.
>>>
>>> The root level error can similarly move to QAPISchema.__init__(), where
>>> we know we'll never have an info context to report with, so we use a
>>> more abstract error type.
>>>
>>> Now the error looks sensible again:
>>>
>>>> python3 qapi-gen.py 'fake.json'
>>> qapi-gen.py: can't read schema file 'fake.json': No such file or directory
>>>
>>> With these error cases separated, QAPISourceInfo can be solidified as
>>> never having placeholder arguments that violate our desired types. Clean
>>> up test-qapi along similar lines.
>>>
>>> Fixes: 52a474180a
>>>
>>> Signed-off-by: John Snow <js...@redhat.com>
[...]
>> Patch looks good to me.
>>
>
> Well, that's good ;)
