| Message ID | 20200331000014.11581-11-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: use python logging | expand |
On 31.03.20 02:00, John Snow wrote: > Minor cleanup for HMP functions; helps with line length and consolidates > HMP helpers through one implementation function. > > Although we are adding a universal toggle to turn QMP logging on or off, > many existing callers to hmp functions don't expect that output to be > logged, which causes quite a few changes in the test output. > > For now, offer a use_log parameter. > > > Typing notes: > > QMPResponse is just an alias for Dict[str, Any]. It holds no special > meanings and it is not a formal subtype of Dict[str, Any]. It is best > thought of as a lexical synonym. > > We may well wish to add stricter subtypes in the future for certain > shapes of data that are not formalized as Python objects, at which point > we can simply retire the alias and allow mypy to more strictly check > usages of the name. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- > 1 file changed, 22 insertions(+), 13 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com> > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index b08bcb87e1..dfc753c319 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py > @@ -37,6 +37,10 @@ > > assert sys.version_info >= (3, 6) > > +# Type Aliases > +QMPResponse = Dict[str, Any] > + > + > faulthandler.enable() > > # This will not work if arguments contain spaces but is necessary if we > @@ -540,25 +544,30 @@ def add_incoming(self, addr): > self._args.append(addr) > return self > > - def pause_drive(self, drive, event=None): > - '''Pause drive r/w operations''' > + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: > + cmd = 'human-monitor-command' > + kwargs = {'command-line': command_line} > + if use_log: > + return self.qmp_log(cmd, **kwargs) > + else: > + return self.qmp(cmd, **kwargs) Hm. I suppose I should take this chance to understand something about mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this really returns QMPResponse. Is there some flag to make it? Like --actually-check-types? (--strict seems, well, overly strict? Like not allowing generics, I don’t see why. Or I suppose for the time being we want to allow untyped definitions, as long as they don’t break type assertions such as it kind of does here...?) Max
Am 31.03.2020 um 12:21 hat Max Reitz geschrieben: > On 31.03.20 02:00, John Snow wrote: > > Minor cleanup for HMP functions; helps with line length and consolidates > > HMP helpers through one implementation function. > > > > Although we are adding a universal toggle to turn QMP logging on or off, > > many existing callers to hmp functions don't expect that output to be > > logged, which causes quite a few changes in the test output. > > > > For now, offer a use_log parameter. > > > > > > Typing notes: > > > > QMPResponse is just an alias for Dict[str, Any]. It holds no special > > meanings and it is not a formal subtype of Dict[str, Any]. It is best > > thought of as a lexical synonym. > > > > We may well wish to add stricter subtypes in the future for certain > > shapes of data that are not formalized as Python objects, at which point > > we can simply retire the alias and allow mypy to more strictly check > > usages of the name. > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- > > 1 file changed, 22 insertions(+), 13 deletions(-) > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > > index b08bcb87e1..dfc753c319 100644 > > --- a/tests/qemu-iotests/iotests.py > > +++ b/tests/qemu-iotests/iotests.py > > @@ -37,6 +37,10 @@ > > > > assert sys.version_info >= (3, 6) > > > > +# Type Aliases > > +QMPResponse = Dict[str, Any] > > + > > + > > faulthandler.enable() > > > > # This will not work if arguments contain spaces but is necessary if we > > @@ -540,25 +544,30 @@ def add_incoming(self, addr): > > self._args.append(addr) > > return self > > > > - def pause_drive(self, drive, event=None): > > - '''Pause drive r/w operations''' > > + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: > > + cmd = 'human-monitor-command' > > + kwargs = {'command-line': command_line} > > + if use_log: > > + return self.qmp_log(cmd, **kwargs) > > + else: > > + return self.qmp(cmd, **kwargs) > > Hm. I suppose I should take this chance to understand something about > mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this > really returns QMPResponse. Is there some flag to make it? Like > --actually-check-types? There is --check-untyped-defs, but I'm not sure if that actually changes the return type of untyped functions from Any to an inferred type. I kind of doubt it. > (--strict seems, well, overly strict? Like not allowing generics, I > don’t see why. Or I suppose for the time being we want to allow untyped > definitions, as long as they don’t break type assertions such as it kind > of does here...?) At least, --strict does actually complain about this one because Any isn't good enough any more (it includes --warn-return-any): iotests.py:560: warning: Returning Any from function declared to return "Dict[str, Any]" iotests.py:560: error: Call to untyped function "qmp_log" in typed context iotests.py:562: warning: Returning Any from function declared to return "Dict[str, Any]" Not sure why you think it doesn't allow generics? I never had problems with that, even in my Python museum. :-) Kevin
On 3/31/20 6:21 AM, Max Reitz wrote: > On 31.03.20 02:00, John Snow wrote: >> Minor cleanup for HMP functions; helps with line length and consolidates >> HMP helpers through one implementation function. >> >> Although we are adding a universal toggle to turn QMP logging on or off, >> many existing callers to hmp functions don't expect that output to be >> logged, which causes quite a few changes in the test output. >> >> For now, offer a use_log parameter. >> >> >> Typing notes: >> >> QMPResponse is just an alias for Dict[str, Any]. It holds no special >> meanings and it is not a formal subtype of Dict[str, Any]. It is best >> thought of as a lexical synonym. >> >> We may well wish to add stricter subtypes in the future for certain >> shapes of data that are not formalized as Python objects, at which point >> we can simply retire the alias and allow mypy to more strictly check >> usages of the name. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- >> 1 file changed, 22 insertions(+), 13 deletions(-) > > Reviewed-by: Max Reitz <mreitz@redhat.com> > >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >> index b08bcb87e1..dfc753c319 100644 >> --- a/tests/qemu-iotests/iotests.py >> +++ b/tests/qemu-iotests/iotests.py >> @@ -37,6 +37,10 @@ >> >> assert sys.version_info >= (3, 6) >> >> +# Type Aliases >> +QMPResponse = Dict[str, Any] >> + >> + >> faulthandler.enable() >> >> # This will not work if arguments contain spaces but is necessary if we >> @@ -540,25 +544,30 @@ def add_incoming(self, addr): >> self._args.append(addr) >> return self >> >> - def pause_drive(self, drive, event=None): >> - '''Pause drive r/w operations''' >> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: >> + cmd = 'human-monitor-command' >> + kwargs = {'command-line': command_line} >> + if use_log: >> + return self.qmp_log(cmd, **kwargs) >> + else: >> + return self.qmp(cmd, **kwargs) > > Hm. I suppose I should take this chance to understand something about > mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this > really returns QMPResponse. Is there some flag to make it? Like > --actually-check-types? > One of --strict's implied options, I'm not sure which. Otherwise, mypy is geared towards a 'gradual typing' discipline. In truth, I'm a little thankful for that because it helps avoid yak shaving marathons. It does mean that sometimes the annotations don't "do anything" yet, apart from offering hints and documentation in e.g. pycharm. Which does mean that sometimes they can be completely wrong... The more we add, the more we'll catch problems. Once this series is dusted I'll try to tackle more conversions for iotests, qmp, etc. I've got a few WIP patches to tackle conversions for tests/qemu-iotests/*.py but I am trying to shepherd this one in first before I go bananas. > (--strict seems, well, overly strict? Like not allowing generics, I > don’t see why. Or I suppose for the time being we want to allow untyped > definitions, as long as they don’t break type assertions such as it kind > of does here...?) > "disallow-any-generics" means disallowing `Any` generics, not disallowing generics ... in general. (I think? I've been using mypy in strict mode for a personal project a lot lately and I use generics in a few places, it seems OK.) > Max >
Am 31.03.2020 um 19:23 hat John Snow geschrieben: > > > On 3/31/20 6:21 AM, Max Reitz wrote: > > On 31.03.20 02:00, John Snow wrote: > >> Minor cleanup for HMP functions; helps with line length and consolidates > >> HMP helpers through one implementation function. > >> > >> Although we are adding a universal toggle to turn QMP logging on or off, > >> many existing callers to hmp functions don't expect that output to be > >> logged, which causes quite a few changes in the test output. > >> > >> For now, offer a use_log parameter. > >> > >> > >> Typing notes: > >> > >> QMPResponse is just an alias for Dict[str, Any]. It holds no special > >> meanings and it is not a formal subtype of Dict[str, Any]. It is best > >> thought of as a lexical synonym. > >> > >> We may well wish to add stricter subtypes in the future for certain > >> shapes of data that are not formalized as Python objects, at which point > >> we can simply retire the alias and allow mypy to more strictly check > >> usages of the name. > >> > >> Signed-off-by: John Snow <jsnow@redhat.com> > >> --- > >> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- > >> 1 file changed, 22 insertions(+), 13 deletions(-) > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > >> index b08bcb87e1..dfc753c319 100644 > >> --- a/tests/qemu-iotests/iotests.py > >> +++ b/tests/qemu-iotests/iotests.py > >> @@ -37,6 +37,10 @@ > >> > >> assert sys.version_info >= (3, 6) > >> > >> +# Type Aliases > >> +QMPResponse = Dict[str, Any] > >> + > >> + > >> faulthandler.enable() > >> > >> # This will not work if arguments contain spaces but is necessary if we > >> @@ -540,25 +544,30 @@ def add_incoming(self, addr): > >> self._args.append(addr) > >> return self > >> > >> - def pause_drive(self, drive, event=None): > >> - '''Pause drive r/w operations''' > >> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: > >> + cmd = 'human-monitor-command' > >> + kwargs = {'command-line': command_line} > >> + if use_log: > >> + return self.qmp_log(cmd, **kwargs) > >> + else: > >> + return self.qmp(cmd, **kwargs) > > > > Hm. I suppose I should take this chance to understand something about > > mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this > > really returns QMPResponse. Is there some flag to make it? Like > > --actually-check-types? > > > > One of --strict's implied options, I'm not sure which. Otherwise, mypy > is geared towards a 'gradual typing' discipline. > > In truth, I'm a little thankful for that because it helps avoid yak > shaving marathons. > > It does mean that sometimes the annotations don't "do anything" yet, > apart from offering hints and documentation in e.g. pycharm. Which does > mean that sometimes they can be completely wrong... > > The more we add, the more we'll catch problems. > > Once this series is dusted I'll try to tackle more conversions for > iotests, qmp, etc. I've got a few WIP patches to tackle conversions for > tests/qemu-iotests/*.py but I am trying to shepherd this one in first > before I go bananas. > > > (--strict seems, well, overly strict? Like not allowing generics, I > > don’t see why. Or I suppose for the time being we want to allow untyped > > definitions, as long as they don’t break type assertions such as it kind > > of does here...?) > > > > "disallow-any-generics" means disallowing `Any` generics, not > disallowing generics ... in general. (I think? I've been using mypy in > strict mode for a personal project a lot lately and I use generics in a > few places, it seems OK.) --disallow-any-generics disallow usage of generic types that do not specify explicit type parameters So it will complain if you say just List, and you need to be explicit if you really want List[Any]. Which I think is a reasonable thing to require. Kevin
On 31.03.20 16:00, Kevin Wolf wrote: > Am 31.03.2020 um 12:21 hat Max Reitz geschrieben: >> On 31.03.20 02:00, John Snow wrote: >>> Minor cleanup for HMP functions; helps with line length and consolidates >>> HMP helpers through one implementation function. >>> >>> Although we are adding a universal toggle to turn QMP logging on or off, >>> many existing callers to hmp functions don't expect that output to be >>> logged, which causes quite a few changes in the test output. >>> >>> For now, offer a use_log parameter. >>> >>> >>> Typing notes: >>> >>> QMPResponse is just an alias for Dict[str, Any]. It holds no special >>> meanings and it is not a formal subtype of Dict[str, Any]. It is best >>> thought of as a lexical synonym. >>> >>> We may well wish to add stricter subtypes in the future for certain >>> shapes of data that are not formalized as Python objects, at which point >>> we can simply retire the alias and allow mypy to more strictly check >>> usages of the name. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- >>> 1 file changed, 22 insertions(+), 13 deletions(-) >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >>> index b08bcb87e1..dfc753c319 100644 >>> --- a/tests/qemu-iotests/iotests.py >>> +++ b/tests/qemu-iotests/iotests.py >>> @@ -37,6 +37,10 @@ >>> >>> assert sys.version_info >= (3, 6) >>> >>> +# Type Aliases >>> +QMPResponse = Dict[str, Any] >>> + >>> + >>> faulthandler.enable() >>> >>> # This will not work if arguments contain spaces but is necessary if we >>> @@ -540,25 +544,30 @@ def add_incoming(self, addr): >>> self._args.append(addr) >>> return self >>> >>> - def pause_drive(self, drive, event=None): >>> - '''Pause drive r/w operations''' >>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: >>> + cmd = 'human-monitor-command' >>> + kwargs = {'command-line': command_line} >>> + if use_log: >>> + return self.qmp_log(cmd, **kwargs) >>> + else: >>> + return self.qmp(cmd, **kwargs) >> >> Hm. I suppose I should take this chance to understand something about >> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this >> really returns QMPResponse. Is there some flag to make it? Like >> --actually-check-types? > > There is --check-untyped-defs, but I'm not sure if that actually changes > the return type of untyped functions from Any to an inferred type. I > kind of doubt it. Well, but Any doesn’t fit into QMPResponse, so there should be an error reported somewhere. >> (--strict seems, well, overly strict? Like not allowing generics, I >> don’t see why. Or I suppose for the time being we want to allow untyped >> definitions, as long as they don’t break type assertions such as it kind >> of does here...?) > > At least, --strict does actually complain about this one because Any > isn't good enough any more (it includes --warn-return-any): Hm, yes, but we’re not at a point where it’s really feasible to enable --strict... > iotests.py:560: warning: Returning Any from function declared to return "Dict[str, Any]" > iotests.py:560: error: Call to untyped function "qmp_log" in typed context > iotests.py:562: warning: Returning Any from function declared to return "Dict[str, Any]" > > Not sure why you think it doesn't allow generics? I never had problems > with that, even in my Python museum. :-) I thought --disallow-any-generics would mean that. But I suppose mypy understands a “generic” to be something else than I do, as John described... *shrug* Max
On 31.03.20 19:39, Kevin Wolf wrote: > Am 31.03.2020 um 19:23 hat John Snow geschrieben: >> >> >> On 3/31/20 6:21 AM, Max Reitz wrote: >>> On 31.03.20 02:00, John Snow wrote: >>>> Minor cleanup for HMP functions; helps with line length and consolidates >>>> HMP helpers through one implementation function. >>>> >>>> Although we are adding a universal toggle to turn QMP logging on or off, >>>> many existing callers to hmp functions don't expect that output to be >>>> logged, which causes quite a few changes in the test output. >>>> >>>> For now, offer a use_log parameter. >>>> >>>> >>>> Typing notes: >>>> >>>> QMPResponse is just an alias for Dict[str, Any]. It holds no special >>>> meanings and it is not a formal subtype of Dict[str, Any]. It is best >>>> thought of as a lexical synonym. >>>> >>>> We may well wish to add stricter subtypes in the future for certain >>>> shapes of data that are not formalized as Python objects, at which point >>>> we can simply retire the alias and allow mypy to more strictly check >>>> usages of the name. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- >>>> 1 file changed, 22 insertions(+), 13 deletions(-) >>> >>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>> >>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >>>> index b08bcb87e1..dfc753c319 100644 >>>> --- a/tests/qemu-iotests/iotests.py >>>> +++ b/tests/qemu-iotests/iotests.py >>>> @@ -37,6 +37,10 @@ >>>> >>>> assert sys.version_info >= (3, 6) >>>> >>>> +# Type Aliases >>>> +QMPResponse = Dict[str, Any] >>>> + >>>> + >>>> faulthandler.enable() >>>> >>>> # This will not work if arguments contain spaces but is necessary if we >>>> @@ -540,25 +544,30 @@ def add_incoming(self, addr): >>>> self._args.append(addr) >>>> return self >>>> >>>> - def pause_drive(self, drive, event=None): >>>> - '''Pause drive r/w operations''' >>>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: >>>> + cmd = 'human-monitor-command' >>>> + kwargs = {'command-line': command_line} >>>> + if use_log: >>>> + return self.qmp_log(cmd, **kwargs) >>>> + else: >>>> + return self.qmp(cmd, **kwargs) >>> >>> Hm. I suppose I should take this chance to understand something about >>> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this >>> really returns QMPResponse. Is there some flag to make it? Like >>> --actually-check-types? >>> >> >> One of --strict's implied options, I'm not sure which. Otherwise, mypy >> is geared towards a 'gradual typing' discipline. >> >> In truth, I'm a little thankful for that because it helps avoid yak >> shaving marathons. Sure. I was just looking into the different options. I was interested in whether I could come up with a mode that leaves wholly untyped code alone, but warns for code that mixes it. Or something. >> It does mean that sometimes the annotations don't "do anything" yet, >> apart from offering hints and documentation in e.g. pycharm. Which does >> mean that sometimes they can be completely wrong... >> >> The more we add, the more we'll catch problems. >> >> Once this series is dusted I'll try to tackle more conversions for >> iotests, qmp, etc. I've got a few WIP patches to tackle conversions for >> tests/qemu-iotests/*.py but I am trying to shepherd this one in first >> before I go bananas. Sure, sure. >>> (--strict seems, well, overly strict? Like not allowing generics, I >>> don’t see why. Or I suppose for the time being we want to allow untyped >>> definitions, as long as they don’t break type assertions such as it kind >>> of does here...?) >>> >> >> "disallow-any-generics" means disallowing `Any` generics, not >> disallowing generics ... in general. (I think? I've been using mypy in >> strict mode for a personal project a lot lately and I use generics in a >> few places, it seems OK.) > > --disallow-any-generics > disallow usage of generic types that do not specify explicit type parameters > > So it will complain if you say just List, and you need to be explicit if > you really want List[Any]. Which I think is a reasonable thing to > require. OK. So it’s “disallow ‘any’ generics”, not “disallow any ‘generic’s”. Not easy to parse. (Yes, yes, I should’ve actually read the man page...) Good to know that mypy and me actually do seem to loosely agree on what a generic is. :) Max
On 01.04.20 14:28, Max Reitz wrote: > On 31.03.20 16:00, Kevin Wolf wrote: >> Am 31.03.2020 um 12:21 hat Max Reitz geschrieben: >>> On 31.03.20 02:00, John Snow wrote: >>>> Minor cleanup for HMP functions; helps with line length and consolidates >>>> HMP helpers through one implementation function. >>>> >>>> Although we are adding a universal toggle to turn QMP logging on or off, >>>> many existing callers to hmp functions don't expect that output to be >>>> logged, which causes quite a few changes in the test output. >>>> >>>> For now, offer a use_log parameter. >>>> >>>> >>>> Typing notes: >>>> >>>> QMPResponse is just an alias for Dict[str, Any]. It holds no special >>>> meanings and it is not a formal subtype of Dict[str, Any]. It is best >>>> thought of as a lexical synonym. >>>> >>>> We may well wish to add stricter subtypes in the future for certain >>>> shapes of data that are not formalized as Python objects, at which point >>>> we can simply retire the alias and allow mypy to more strictly check >>>> usages of the name. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- >>>> 1 file changed, 22 insertions(+), 13 deletions(-) >>> >>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>> >>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >>>> index b08bcb87e1..dfc753c319 100644 >>>> --- a/tests/qemu-iotests/iotests.py >>>> +++ b/tests/qemu-iotests/iotests.py >>>> @@ -37,6 +37,10 @@ >>>> >>>> assert sys.version_info >= (3, 6) >>>> >>>> +# Type Aliases >>>> +QMPResponse = Dict[str, Any] >>>> + >>>> + >>>> faulthandler.enable() >>>> >>>> # This will not work if arguments contain spaces but is necessary if we >>>> @@ -540,25 +544,30 @@ def add_incoming(self, addr): >>>> self._args.append(addr) >>>> return self >>>> >>>> - def pause_drive(self, drive, event=None): >>>> - '''Pause drive r/w operations''' >>>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: >>>> + cmd = 'human-monitor-command' >>>> + kwargs = {'command-line': command_line} >>>> + if use_log: >>>> + return self.qmp_log(cmd, **kwargs) >>>> + else: >>>> + return self.qmp(cmd, **kwargs) >>> >>> Hm. I suppose I should take this chance to understand something about >>> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this >>> really returns QMPResponse. Is there some flag to make it? Like >>> --actually-check-types? >> >> There is --check-untyped-defs, but I'm not sure if that actually changes >> the return type of untyped functions from Any to an inferred type. I >> kind of doubt it. > > Well, but Any doesn’t fit into QMPResponse, so there should be an error > reported somewhere. > >>> (--strict seems, well, overly strict? Like not allowing generics, I >>> don’t see why. Or I suppose for the time being we want to allow untyped >>> definitions, as long as they don’t break type assertions such as it kind >>> of does here...?) >> >> At least, --strict does actually complain about this one because Any >> isn't good enough any more (it includes --warn-return-any): > > Hm, yes, but we’re not at a point where it’s really feasible to enable > --strict... > >> iotests.py:560: warning: Returning Any from function declared to return "Dict[str, Any]" >> iotests.py:560: error: Call to untyped function "qmp_log" in typed context >> iotests.py:562: warning: Returning Any from function declared to return "Dict[str, Any]" >> >> Not sure why you think it doesn't allow generics? I never had problems >> with that, even in my Python museum. :-) > > I thought --disallow-any-generics would mean that. But I suppose mypy > understands a “generic” to be something else than I do, as John > described... *shrug* Oh. John didn’t describe that. I just read the “Any” thing wrong, again. (On my first read, I thought he just used the back ticks to stress the word “any”, not to refer to the type “Any”.) Max
Am 01.04.2020 um 14:28 hat Max Reitz geschrieben: > On 31.03.20 16:00, Kevin Wolf wrote: > > Am 31.03.2020 um 12:21 hat Max Reitz geschrieben: > >> On 31.03.20 02:00, John Snow wrote: > >>> Minor cleanup for HMP functions; helps with line length and consolidates > >>> HMP helpers through one implementation function. > >>> > >>> Although we are adding a universal toggle to turn QMP logging on or off, > >>> many existing callers to hmp functions don't expect that output to be > >>> logged, which causes quite a few changes in the test output. > >>> > >>> For now, offer a use_log parameter. > >>> > >>> > >>> Typing notes: > >>> > >>> QMPResponse is just an alias for Dict[str, Any]. It holds no special > >>> meanings and it is not a formal subtype of Dict[str, Any]. It is best > >>> thought of as a lexical synonym. > >>> > >>> We may well wish to add stricter subtypes in the future for certain > >>> shapes of data that are not formalized as Python objects, at which point > >>> we can simply retire the alias and allow mypy to more strictly check > >>> usages of the name. > >>> > >>> Signed-off-by: John Snow <jsnow@redhat.com> > >>> --- > >>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- > >>> 1 file changed, 22 insertions(+), 13 deletions(-) > >> > >> Reviewed-by: Max Reitz <mreitz@redhat.com> > >> > >>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > >>> index b08bcb87e1..dfc753c319 100644 > >>> --- a/tests/qemu-iotests/iotests.py > >>> +++ b/tests/qemu-iotests/iotests.py > >>> @@ -37,6 +37,10 @@ > >>> > >>> assert sys.version_info >= (3, 6) > >>> > >>> +# Type Aliases > >>> +QMPResponse = Dict[str, Any] > >>> + > >>> + > >>> faulthandler.enable() > >>> > >>> # This will not work if arguments contain spaces but is necessary if we > >>> @@ -540,25 +544,30 @@ def add_incoming(self, addr): > >>> self._args.append(addr) > >>> return self > >>> > >>> - def pause_drive(self, drive, event=None): > >>> - '''Pause drive r/w operations''' > >>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: > >>> + cmd = 'human-monitor-command' > >>> + kwargs = {'command-line': command_line} > >>> + if use_log: > >>> + return self.qmp_log(cmd, **kwargs) > >>> + else: > >>> + return self.qmp(cmd, **kwargs) > >> > >> Hm. I suppose I should take this chance to understand something about > >> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this > >> really returns QMPResponse. Is there some flag to make it? Like > >> --actually-check-types? > > > > There is --check-untyped-defs, but I'm not sure if that actually changes > > the return type of untyped functions from Any to an inferred type. I > > kind of doubt it. > > Well, but Any doesn’t fit into QMPResponse, so there should be an error > reported somewhere. Any is the void* of Python typing. It's compatible with everything else, in both directions. > >> (--strict seems, well, overly strict? Like not allowing generics, I > >> don’t see why. Or I suppose for the time being we want to allow untyped > >> definitions, as long as they don’t break type assertions such as it kind > >> of does here...?) > > > > At least, --strict does actually complain about this one because Any > > isn't good enough any more (it includes --warn-return-any): > > Hm, yes, but we’re not at a point where it’s really feasible to enable > --strict... We're not yet, but I think it's a reasonable goal. Kevin
On 01.04.20 15:51, Kevin Wolf wrote: > Am 01.04.2020 um 14:28 hat Max Reitz geschrieben: >> On 31.03.20 16:00, Kevin Wolf wrote: >>> Am 31.03.2020 um 12:21 hat Max Reitz geschrieben: >>>> On 31.03.20 02:00, John Snow wrote: >>>>> Minor cleanup for HMP functions; helps with line length and consolidates >>>>> HMP helpers through one implementation function. >>>>> >>>>> Although we are adding a universal toggle to turn QMP logging on or off, >>>>> many existing callers to hmp functions don't expect that output to be >>>>> logged, which causes quite a few changes in the test output. >>>>> >>>>> For now, offer a use_log parameter. >>>>> >>>>> >>>>> Typing notes: >>>>> >>>>> QMPResponse is just an alias for Dict[str, Any]. It holds no special >>>>> meanings and it is not a formal subtype of Dict[str, Any]. It is best >>>>> thought of as a lexical synonym. >>>>> >>>>> We may well wish to add stricter subtypes in the future for certain >>>>> shapes of data that are not formalized as Python objects, at which point >>>>> we can simply retire the alias and allow mypy to more strictly check >>>>> usages of the name. >>>>> >>>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>>> --- >>>>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- >>>>> 1 file changed, 22 insertions(+), 13 deletions(-) >>>> >>>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>>> >>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >>>>> index b08bcb87e1..dfc753c319 100644 >>>>> --- a/tests/qemu-iotests/iotests.py >>>>> +++ b/tests/qemu-iotests/iotests.py >>>>> @@ -37,6 +37,10 @@ >>>>> >>>>> assert sys.version_info >= (3, 6) >>>>> >>>>> +# Type Aliases >>>>> +QMPResponse = Dict[str, Any] >>>>> + >>>>> + >>>>> faulthandler.enable() >>>>> >>>>> # This will not work if arguments contain spaces but is necessary if we >>>>> @@ -540,25 +544,30 @@ def add_incoming(self, addr): >>>>> self._args.append(addr) >>>>> return self >>>>> >>>>> - def pause_drive(self, drive, event=None): >>>>> - '''Pause drive r/w operations''' >>>>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: >>>>> + cmd = 'human-monitor-command' >>>>> + kwargs = {'command-line': command_line} >>>>> + if use_log: >>>>> + return self.qmp_log(cmd, **kwargs) >>>>> + else: >>>>> + return self.qmp(cmd, **kwargs) >>>> >>>> Hm. I suppose I should take this chance to understand something about >>>> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this >>>> really returns QMPResponse. Is there some flag to make it? Like >>>> --actually-check-types? >>> >>> There is --check-untyped-defs, but I'm not sure if that actually changes >>> the return type of untyped functions from Any to an inferred type. I >>> kind of doubt it. >> >> Well, but Any doesn’t fit into QMPResponse, so there should be an error >> reported somewhere. > > Any is the void* of Python typing. It's compatible with everything else, > in both directions. void* is handled differently by C and by C++. Max
On 4/1/20 8:40 AM, Max Reitz wrote: > On 31.03.20 19:39, Kevin Wolf wrote: >> Am 31.03.2020 um 19:23 hat John Snow geschrieben: >>> >>> >>> On 3/31/20 6:21 AM, Max Reitz wrote: >>>> On 31.03.20 02:00, John Snow wrote: >>>>> Minor cleanup for HMP functions; helps with line length and consolidates >>>>> HMP helpers through one implementation function. >>>>> >>>>> Although we are adding a universal toggle to turn QMP logging on or off, >>>>> many existing callers to hmp functions don't expect that output to be >>>>> logged, which causes quite a few changes in the test output. >>>>> >>>>> For now, offer a use_log parameter. >>>>> >>>>> >>>>> Typing notes: >>>>> >>>>> QMPResponse is just an alias for Dict[str, Any]. It holds no special >>>>> meanings and it is not a formal subtype of Dict[str, Any]. It is best >>>>> thought of as a lexical synonym. >>>>> >>>>> We may well wish to add stricter subtypes in the future for certain >>>>> shapes of data that are not formalized as Python objects, at which point >>>>> we can simply retire the alias and allow mypy to more strictly check >>>>> usages of the name. >>>>> >>>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>>> --- >>>>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- >>>>> 1 file changed, 22 insertions(+), 13 deletions(-) >>>> >>>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>>> >>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >>>>> index b08bcb87e1..dfc753c319 100644 >>>>> --- a/tests/qemu-iotests/iotests.py >>>>> +++ b/tests/qemu-iotests/iotests.py >>>>> @@ -37,6 +37,10 @@ >>>>> >>>>> assert sys.version_info >= (3, 6) >>>>> >>>>> +# Type Aliases >>>>> +QMPResponse = Dict[str, Any] >>>>> + >>>>> + >>>>> faulthandler.enable() >>>>> >>>>> # This will not work if arguments contain spaces but is necessary if we >>>>> @@ -540,25 +544,30 @@ def add_incoming(self, addr): >>>>> self._args.append(addr) >>>>> return self >>>>> >>>>> - def pause_drive(self, drive, event=None): >>>>> - '''Pause drive r/w operations''' >>>>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: >>>>> + cmd = 'human-monitor-command' >>>>> + kwargs = {'command-line': command_line} >>>>> + if use_log: >>>>> + return self.qmp_log(cmd, **kwargs) >>>>> + else: >>>>> + return self.qmp(cmd, **kwargs) >>>> >>>> Hm. I suppose I should take this chance to understand something about >>>> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this >>>> really returns QMPResponse. Is there some flag to make it? Like >>>> --actually-check-types? >>>> >>> >>> One of --strict's implied options, I'm not sure which. Otherwise, mypy >>> is geared towards a 'gradual typing' discipline. >>> >>> In truth, I'm a little thankful for that because it helps avoid yak >>> shaving marathons. > > Sure. I was just looking into the different options. I was interested > in whether I could come up with a mode that leaves wholly untyped code > alone, but warns for code that mixes it. Or something. > >>> It does mean that sometimes the annotations don't "do anything" yet, >>> apart from offering hints and documentation in e.g. pycharm. Which does >>> mean that sometimes they can be completely wrong... >>> >>> The more we add, the more we'll catch problems. >>> >>> Once this series is dusted I'll try to tackle more conversions for >>> iotests, qmp, etc. I've got a few WIP patches to tackle conversions for >>> tests/qemu-iotests/*.py but I am trying to shepherd this one in first >>> before I go bananas. > > Sure, sure. > >>>> (--strict seems, well, overly strict? Like not allowing generics, I >>>> don’t see why. Or I suppose for the time being we want to allow untyped >>>> definitions, as long as they don’t break type assertions such as it kind >>>> of does here...?) >>>> >>> >>> "disallow-any-generics" means disallowing `Any` generics, not >>> disallowing generics ... in general. (I think? I've been using mypy in >>> strict mode for a personal project a lot lately and I use generics in a >>> few places, it seems OK.) >> >> --disallow-any-generics >> disallow usage of generic types that do not specify explicit type parameters >> >> So it will complain if you say just List, and you need to be explicit if >> you really want List[Any]. Which I think is a reasonable thing to >> require. > > OK. So it’s “disallow ‘any’ generics”, not “disallow any ‘generic’s”. > Not easy to parse. (Yes, yes, I should’ve actually read the man page...) > > Good to know that mypy and me actually do seem to loosely agree on what > a generic is. :) > > Max > Are we squared up for this series? I am actually not sure. --js
Am 02.04.2020 um 20:27 hat John Snow geschrieben: > On 4/1/20 8:40 AM, Max Reitz wrote: > > On 31.03.20 19:39, Kevin Wolf wrote: > >> Am 31.03.2020 um 19:23 hat John Snow geschrieben: > >>> > >>> > >>> On 3/31/20 6:21 AM, Max Reitz wrote: > >>>> On 31.03.20 02:00, John Snow wrote: > >>>>> Minor cleanup for HMP functions; helps with line length and consolidates > >>>>> HMP helpers through one implementation function. > >>>>> > >>>>> Although we are adding a universal toggle to turn QMP logging on or off, > >>>>> many existing callers to hmp functions don't expect that output to be > >>>>> logged, which causes quite a few changes in the test output. > >>>>> > >>>>> For now, offer a use_log parameter. > >>>>> > >>>>> > >>>>> Typing notes: > >>>>> > >>>>> QMPResponse is just an alias for Dict[str, Any]. It holds no special > >>>>> meanings and it is not a formal subtype of Dict[str, Any]. It is best > >>>>> thought of as a lexical synonym. > >>>>> > >>>>> We may well wish to add stricter subtypes in the future for certain > >>>>> shapes of data that are not formalized as Python objects, at which point > >>>>> we can simply retire the alias and allow mypy to more strictly check > >>>>> usages of the name. > >>>>> > >>>>> Signed-off-by: John Snow <jsnow@redhat.com> > >>>>> --- > >>>>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- > >>>>> 1 file changed, 22 insertions(+), 13 deletions(-) > >>>> > >>>> Reviewed-by: Max Reitz <mreitz@redhat.com> > >>>> > >>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > >>>>> index b08bcb87e1..dfc753c319 100644 > >>>>> --- a/tests/qemu-iotests/iotests.py > >>>>> +++ b/tests/qemu-iotests/iotests.py > >>>>> @@ -37,6 +37,10 @@ > >>>>> > >>>>> assert sys.version_info >= (3, 6) > >>>>> > >>>>> +# Type Aliases > >>>>> +QMPResponse = Dict[str, Any] > >>>>> + > >>>>> + > >>>>> faulthandler.enable() > >>>>> > >>>>> # This will not work if arguments contain spaces but is necessary if we > >>>>> @@ -540,25 +544,30 @@ def add_incoming(self, addr): > >>>>> self._args.append(addr) > >>>>> return self > >>>>> > >>>>> - def pause_drive(self, drive, event=None): > >>>>> - '''Pause drive r/w operations''' > >>>>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: > >>>>> + cmd = 'human-monitor-command' > >>>>> + kwargs = {'command-line': command_line} > >>>>> + if use_log: > >>>>> + return self.qmp_log(cmd, **kwargs) > >>>>> + else: > >>>>> + return self.qmp(cmd, **kwargs) > >>>> > >>>> Hm. I suppose I should take this chance to understand something about > >>>> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this > >>>> really returns QMPResponse. Is there some flag to make it? Like > >>>> --actually-check-types? > >>>> > >>> > >>> One of --strict's implied options, I'm not sure which. Otherwise, mypy > >>> is geared towards a 'gradual typing' discipline. > >>> > >>> In truth, I'm a little thankful for that because it helps avoid yak > >>> shaving marathons. > > > > Sure. I was just looking into the different options. I was interested > > in whether I could come up with a mode that leaves wholly untyped code > > alone, but warns for code that mixes it. Or something. > > > >>> It does mean that sometimes the annotations don't "do anything" yet, > >>> apart from offering hints and documentation in e.g. pycharm. Which does > >>> mean that sometimes they can be completely wrong... > >>> > >>> The more we add, the more we'll catch problems. > >>> > >>> Once this series is dusted I'll try to tackle more conversions for > >>> iotests, qmp, etc. I've got a few WIP patches to tackle conversions for > >>> tests/qemu-iotests/*.py but I am trying to shepherd this one in first > >>> before I go bananas. > > > > Sure, sure. > > > >>>> (--strict seems, well, overly strict? Like not allowing generics, I > >>>> don’t see why. Or I suppose for the time being we want to allow untyped > >>>> definitions, as long as they don’t break type assertions such as it kind > >>>> of does here...?) > >>>> > >>> > >>> "disallow-any-generics" means disallowing `Any` generics, not > >>> disallowing generics ... in general. (I think? I've been using mypy in > >>> strict mode for a personal project a lot lately and I use generics in a > >>> few places, it seems OK.) > >> > >> --disallow-any-generics > >> disallow usage of generic types that do not specify explicit type parameters > >> > >> So it will complain if you say just List, and you need to be explicit if > >> you really want List[Any]. Which I think is a reasonable thing to > >> require. > > > > OK. So it’s “disallow ‘any’ generics”, not “disallow any ‘generic’s”. > > Not easy to parse. (Yes, yes, I should’ve actually read the man page...) > > > > Good to know that mypy and me actually do seem to loosely agree on what > > a generic is. :) > > > > Max > > > > Are we squared up for this series? I am actually not sure. I had a comment in patch 14 which you may or may not want to address (my R-b was unconditional). I think everything else was just tangential discussion. Kevin
On 02.04.20 20:27, John Snow wrote:
[...]
> Are we squared up for this series? I am actually not sure.
As far as I’m concerned, yes. I just had this question on how to use mypy.
Max
On 4/3/20 4:57 AM, Max Reitz wrote: > On 02.04.20 20:27, John Snow wrote: > > [...] > >> Are we squared up for this series? I am actually not sure. > > As far as I’m concerned, yes. I just had this question on how to use mypy. Oh, whoops, Kevin's comment. I do want to address that one. Sorry, I am a bit distracted right now. The last few weeks have been tough. Having a hard time keeping track of things right now. --js
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index b08bcb87e1..dfc753c319 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -37,6 +37,10 @@ assert sys.version_info >= (3, 6) +# Type Aliases +QMPResponse = Dict[str, Any] + + faulthandler.enable() # This will not work if arguments contain spaces but is necessary if we @@ -540,25 +544,30 @@ def add_incoming(self, addr): self._args.append(addr) return self - def pause_drive(self, drive, event=None): - '''Pause drive r/w operations''' + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse: + cmd = 'human-monitor-command' + kwargs = {'command-line': command_line} + if use_log: + return self.qmp_log(cmd, **kwargs) + else: + return self.qmp(cmd, **kwargs) + + def pause_drive(self, drive: str, event: Optional[str] = None) -> None: + """Pause drive r/w operations""" if not event: self.pause_drive(drive, "read_aio") self.pause_drive(drive, "write_aio") return - self.qmp('human-monitor-command', - command_line='qemu-io %s "break %s bp_%s"' - % (drive, event, drive)) + self.hmp(f'qemu-io {drive} "break {event} bp_{drive}"') - def resume_drive(self, drive): - self.qmp('human-monitor-command', - command_line='qemu-io %s "remove_break bp_%s"' - % (drive, drive)) + def resume_drive(self, drive: str) -> None: + """Resume drive r/w operations""" + self.hmp(f'qemu-io {drive} "remove_break bp_{drive}"') - def hmp_qemu_io(self, drive, cmd): - '''Write to a given drive using an HMP command''' - return self.qmp('human-monitor-command', - command_line='qemu-io %s "%s"' % (drive, cmd)) + def hmp_qemu_io(self, drive: str, cmd: str, + use_log: bool = False) -> QMPResponse: + """Write to a given drive using an HMP command""" + return self.hmp(f'qemu-io {drive} "{cmd}"', use_log=use_log) def flatten_qmp_object(self, obj, output=None, basestr=''): if output is None:
Minor cleanup for HMP functions; helps with line length and consolidates HMP helpers through one implementation function. Although we are adding a universal toggle to turn QMP logging on or off, many existing callers to hmp functions don't expect that output to be logged, which causes quite a few changes in the test output. For now, offer a use_log parameter. Typing notes: QMPResponse is just an alias for Dict[str, Any]. It holds no special meanings and it is not a formal subtype of Dict[str, Any]. It is best thought of as a lexical synonym. We may well wish to add stricter subtypes in the future for certain shapes of data that are not formalized as Python objects, at which point we can simply retire the alias and allow mypy to more strictly check usages of the name. Signed-off-by: John Snow <jsnow@redhat.com> --- tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)