-
Notifications
You must be signed in to change notification settings - Fork 84
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
Fix string format "blabla" "bla"
#1243
Conversation
To me if you did |
This reverts commit 60406f0.
"blabla" "bla"
OK, I did not notice this was using In an ideal world, we would get both the caller line and the line where the warning was emitted (stacklevel=1, to quickly jump to the warning line and figure out why it was emitted in the first place). I use this second option very often. Anyway, for now I removed that erroneous |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1243 +/- ##
==========================================
- Coverage 97.61% 97.60% -0.02%
==========================================
Files 40 40
Lines 8685 8682 -3
==========================================
- Hits 8478 8474 -4
- Misses 207 208 +1 ☔ View full report in Codecov by Sentry. |
In theory it should be a few lines to add an
then mne-bids could make use of it. Maybe we'd even want |
@larsoner That's a nice idea, and would give the With that proposition, the caller line would not benefit from the same 'link' to jump to the definition; and thus without this I don't see the added benefit compared to quickly searching for the error string within the codebase. Anyway, I will give it a try and see how it renders ;) |
Let's keep this PR as a simple string format fix then, mark for merge if happy :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mscheltienne!
PR Description
I propose to change the
stacklevel
of a couple of warnings I encountered, becauseis really not that informative and I would appreciate to quickly jump to the function issuing the warning.
While I was on it, I fixed probably all the occurrences of
"blabla" "bla"
strings.I don't believe this minor maintenance PR needs a changelog entry.
Merge checklist
Maintainer, please confirm the following before merging.
If applicable: