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.
Thanks, and apologies for the fastball.