| Message ID | 20231004194613.2900323-2-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,1/9] Python/iotests: Add type hint for nbd module | expand |
On Wed, Oct 04, 2023 at 03:46:05PM -0400, John Snow wrote: > The test bails gracefully if this module isn't installed, but linters > need a little help understanding that. It's enough to just declare the > type in this case. > > (Fixes pylint complaining about use of an uninitialized variable because > it isn't wise enough to understand the notrun call is noreturn.) > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > tests/qemu-iotests/tests/nbd-multiconn | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Since there's questions about this pull request seeming to be the first time this patch has appeared on list, I'll go ahead and review it here on the assumption that a v2 pull request is warranted. > > diff --git a/tests/qemu-iotests/tests/nbd-multiconn b/tests/qemu-iotests/tests/nbd-multiconn > index 478a1eaba27..7e686a786ea 100755 > --- a/tests/qemu-iotests/tests/nbd-multiconn > +++ b/tests/qemu-iotests/tests/nbd-multiconn > @@ -20,6 +20,8 @@ > > import os > from contextlib import contextmanager > +from types import ModuleType > + > import iotests > from iotests import qemu_img_create, qemu_io > > @@ -28,7 +30,7 @@ disk = os.path.join(iotests.test_dir, 'disk') > size = '4M' > nbd_sock = os.path.join(iotests.sock_dir, 'nbd_sock') > nbd_uri = 'nbd+unix:///{}?socket=' + nbd_sock > - > +nbd: ModuleType Is it possible to put this closer to the code actually using 'nbd', as in this region? | if __name__ == '__main__': | try: | # Easier to use libnbd than to try and set up parallel | # 'qemu-nbd --list' or 'qemu-io' processes, but not all systems | # have libnbd installed. | import nbd # type: ignore but then again, open_nbd() right after your current location utilizes the variable, so I guess not. I trust your judgment on silencing the linters, so whether or not you move it (if moving is even possible), Reviewed-by: Eric Blake <eblake@redhat.com>
On Thu, Oct 5, 2023, 10:05 AM Eric Blake <eblake@redhat.com> wrote: > On Wed, Oct 04, 2023 at 03:46:05PM -0400, John Snow wrote: > > The test bails gracefully if this module isn't installed, but linters > > need a little help understanding that. It's enough to just declare the > > type in this case. > > > > (Fixes pylint complaining about use of an uninitialized variable because > > it isn't wise enough to understand the notrun call is noreturn.) > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > tests/qemu-iotests/tests/nbd-multiconn | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > Since there's questions about this pull request seeming to be the > first time this patch has appeared on list, I'll go ahead and review > it here on the assumption that a v2 pull request is warranted. > Sorry... Tried to "sneak" in some real trivial stuff, but didn't mean to try and pull a fast one. I figured it'd get reviewed and then we'd merge. I can see this sentiment is not a well expected one 😓 > > > > diff --git a/tests/qemu-iotests/tests/nbd-multiconn > b/tests/qemu-iotests/tests/nbd-multiconn > > index 478a1eaba27..7e686a786ea 100755 > > --- a/tests/qemu-iotests/tests/nbd-multiconn > > +++ b/tests/qemu-iotests/tests/nbd-multiconn > > @@ -20,6 +20,8 @@ > > > > import os > > from contextlib import contextmanager > > +from types import ModuleType > > + > > import iotests > > from iotests import qemu_img_create, qemu_io > > > > @@ -28,7 +30,7 @@ disk = os.path.join(iotests.test_dir, 'disk') > > size = '4M' > > nbd_sock = os.path.join(iotests.sock_dir, 'nbd_sock') > > nbd_uri = 'nbd+unix:///{}?socket=' + nbd_sock > > - > > +nbd: ModuleType > > Is it possible to put this closer to the code actually using 'nbd', as > in this region? > > | if __name__ == '__main__': > | try: > | # Easier to use libnbd than to try and set up parallel > | # 'qemu-nbd --list' or 'qemu-io' processes, but not all systems > | # have libnbd installed. > | import nbd # type: ignore > > but then again, open_nbd() right after your current location utilizes > the variable, so I guess not. I trust your judgment on silencing the > linters, so whether or not you move it (if moving is even possible), > It might be possible to move things around to be more localized, but this was the smallest possible diffstat. It's not really about the type, the declaration also helps pylint not complain the "potentially" illegal use. (type: ignore isn't sufficient.) The alternative is, I think, using some try...except around the import up at the top, and using a HAVE_NBDLIB variable that we use to exit early instead. I think there's no real benefit to that much churn, and this gets the job done with less. It might be possible to teach pylint that notrun is a NORETURN function, but I didn't explore that avenue. > Reviewed-by: Eric Blake <eblake@redhat.com> > Thanks, and apologies for the fastball. > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. > Virtualization: qemu.org | libguestfs.org > >
diff --git a/tests/qemu-iotests/tests/nbd-multiconn b/tests/qemu-iotests/tests/nbd-multiconn index 478a1eaba27..7e686a786ea 100755 --- a/tests/qemu-iotests/tests/nbd-multiconn +++ b/tests/qemu-iotests/tests/nbd-multiconn @@ -20,6 +20,8 @@ import os from contextlib import contextmanager +from types import ModuleType + import iotests from iotests import qemu_img_create, qemu_io @@ -28,7 +30,7 @@ disk = os.path.join(iotests.test_dir, 'disk') size = '4M' nbd_sock = os.path.join(iotests.sock_dir, 'nbd_sock') nbd_uri = 'nbd+unix:///{}?socket=' + nbd_sock - +nbd: ModuleType @contextmanager def open_nbd(export_name):
The test bails gracefully if this module isn't installed, but linters need a little help understanding that. It's enough to just declare the type in this case. (Fixes pylint complaining about use of an uninitialized variable because it isn't wise enough to understand the notrun call is noreturn.) Signed-off-by: John Snow <jsnow@redhat.com> --- tests/qemu-iotests/tests/nbd-multiconn | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)