Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xcp/cpiofile.py: Fix warning: Reimport 'bz2' (imported line 48) #58

Merged

Conversation

bernhardkaindl
Copy link
Collaborator

@bernhardkaindl bernhardkaindl commented May 31, 2023

Two weeks ago when adding cast()s to bz2 types,
I merged a toplevel import bz2 because in currently used Python builds bz2 is built-in.

It's time to remove the non-toplevel import bz2 lines which now trigger several warnings:

"Reimport 'bz2' (imported line 48)"`

@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #58 (f2c6ebc) into master (cdaa7bf) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
+ Coverage   70.90%   70.97%   +0.06%     
==========================================
  Files          21       21              
  Lines        3471     3462       -9     
==========================================
- Hits         2461     2457       -4     
+ Misses       1010     1005       -5     
Flag Coverage Δ
unittest 70.97% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
xcp/cpiofile.py 64.90% <ø> (+0.16%) ⬆️
xcp/pci.py 79.78% <ø> (ø)

Two weeks ago when adding cast()s to bz2 types, I merged a toplevel
`import bz2` because in currently used Python builds bz2 is built-in.

It's time to remove the non-toplevel `import bz2` lines which
now trigger several warnings: "Reimport 'bz2' (imported line 48)"

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
@bernhardkaindl bernhardkaindl requested a review from psafont May 31, 2023 17:14
Copy link
Collaborator Author

@bernhardkaindl bernhardkaindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psafont Pushed a backout of the change to the file mode and the shebang line.

Currently, 19 of the 46 *.py files have the shebang and it's not a problem to have it,
it should just be superflous, I believe, but that's not for now.

@bernhardkaindl bernhardkaindl merged commit 537c762 into xenserver:master Jun 1, 2023
bernhardkaindl added a commit to xenserver-next/python-libs that referenced this pull request Jun 14, 2023
As discussed in xenserver#58:

Only files which will be execute by invoking them directly require
the shebang. If a library module would be used as a script as well,
the file could have to contain a __name__ guard to prevent it to
be executed on importing it:

if __name__ == "__main__":
    main()

None of the files in xcp/* have such __name__ guard, hence we can
safely cleanup the excess shebangs in xcp. Even the copyright headers
in the otherwise empty __init__.py files have the shebangs, so it looks
like the shebangs were just added as part of the copyright header.

It could have been used as a marker for editors to switch to
Python mode, about stackoverflow agrees that that's not needed.

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
bernhardkaindl added a commit to xenserver-next/python-libs that referenced this pull request Jun 15, 2023
As discussed in xenserver#58:

Only files which will be execute by invoking them directly require
the shebang. If a library module would be used as a script as well,
the file could have to contain a __name__ guard to prevent it to
be executed on importing it:

if __name__ == "__main__":
    main()

None of the files in xcp/* have such __name__ guard, hence we can
safely cleanup the excess shebangs in xcp. Even the copyright headers
in the otherwise empty __init__.py files have the shebangs, so it looks
like the shebangs were just added as part of the copyright header.

It could have been used as a marker for editors to switch to
Python mode, about stackoverflow agrees that that's not needed.

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants