MAINT: Add explicit level to warn. #5649
Conversation
Carreau
changed the title
|
It is not all warnings pointed by pre-commit. Is it intentional? |
|
Yes. It's to make review and merging easier, and decrease the chances of conflict with other PRs and make the chances of it getting merge a bit higher. I don't think many reviewers want to have a PR that touch 35 files and have 70+ updates like this. |
| @@ -58,7 +58,7 @@ | |||
| deferred=True, | |||
| version=QtCore.__version__, | |||
| ) | |||
| warn(message=warn_message) | |||
There was a problem hiding this comment.
why stacklevel=1 when it is executed on import so it will be nice to have information where import happen?
There was a problem hiding this comment.
My understanding was that stacklevel does not work on import and ends up in the CPython import machinery.
I'm also unsure the error message would be useful as it would not point to an actual error the user is doing on the line of the import, but I can still put 2.
| stacklevel=1, | ||
| ) | ||
| self._console = None | ||
| except ImportError: | ||
| traceback.print_exc() | ||
| warnings.warn( | ||
| trans._( | ||
| 'error importing napari-console. See console for full error.' | ||
| ) | ||
| ), | ||
| stacklevel=1, |
There was a problem hiding this comment.
Maybe stacklevel=2? But I'm not sure.
There was a problem hiding this comment.
I put 1 as it's property and pointing at the line with the property might be unclear. But sure.
Codecov Report
@@ Coverage Diff @@
## main #5649 +/- ##
==========================================
+ Coverage 88.37% 89.75% +1.37%
==========================================
Files 611 611
Lines 51685 51558 -127
==========================================
+ Hits 45679 46275 +596
+ Misses 6006 5283 -723
... and 93 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| stacklevel=1, | ||
| ) | ||
| self._console = None | ||
| except ImportError: | ||
| traceback.print_exc() | ||
| warnings.warn( | ||
| trans._( | ||
| 'error importing napari-console. See console for full error.' | ||
| ) | ||
| ), | ||
| stacklevel=1, |
There was a problem hiding this comment.
I put 1 as it's property and pointing at the line with the property might be unclear. But sure.
|
Maybe merge main branch to pr to remove changes not related to PR? |
github-actions
bot
added
the
preferences
Step toward napari#5645 only touching a few file to make review easier
Yeah, github's update with rebase button is broken. I had to re-rebase manually a bunch of branches. |
Carreau
removed
the
preferences
Czaki
removed
the
ready to merge
|
Thanks ! |
Step toward #5645 only touching a few file to make review easier New pre-commit configuration (new version of ruff) requires warnings to have a stacklevel.
Step toward #5645 only touching a few file to make review easier New pre-commit configuration (new version of ruff) requires warnings to have a stacklevel.
Step toward #5645 only touching a few file to make review easier New pre-commit configuration (new version of ruff) requires warnings to have a stacklevel.
Step toward #5645 only touching a few file to make review easier New pre-commit configuration (new version of ruff) requires warnings to have a stacklevel.
Step toward #5645 only touching a few file to make review easier
New pre-commit configuration (new version of ruff) requires warnings to have a stacklevel.