fix(bst): Do not alter logger in library mode, log to stderr not stdout#2085
Merged
effigies merged 3 commits intobids-standard:maint/1.10.0from Mar 13, 2025
Merged
fix(bst): Do not alter logger in library mode, log to stderr not stdout#2085effigies merged 3 commits intobids-standard:maint/1.10.0from
effigies merged 3 commits intobids-standard:maint/1.10.0from
Conversation
yarikoptic
added
the
schema-code
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2085 +/- ##
==========================================
- Coverage 83.49% 82.92% -0.58%
==========================================
Files 17 17
Lines 1509 1511 +2
==========================================
- Hits 1260 1253 -7
- Misses 249 258 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
effigies
requested review from
DimitriPapadopoulos,
effigies,
ezemikulan,
sappelhoff and
tsalo
as code owners
Libraries should not set handlers or even change log level (by default). It is up to the user-facing code (project using the library, e.g. CLI) to set the logging etc. Otherwise libraries would interfere with desired behaviors of the project, as they do now here with logging polluting stdout of a tool which must produce yaml output
effigies
removed request for
DimitriPapadopoulos,
erdalkaraca,
ezemikulan,
sappelhoff and
tsalo
Collaborator
|
Thanks, if this works for your use case, that's great. I don't have a great feel for logging best practices for libraries vs apps, so I haven't imposed much of a structure here. As far as I'm concerned, you're welcome to refactor this entirely, if you have further suggestions. Do you have a doc that you have written or use as a guideline? It would be nice to have a reference I can go back to. |
effigies
added
the
exclude-from-changelog
effigies
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We tried to upgrade to use recent bidsschematools in dandi-cli (dandi/dandi-cli#1591) but tests started to fail since output was polluted with log messages from bidsschematools, which prompted me to look inside.
I have tried to keep my changes minimal (but did rename
loggertolgrfor consistency) and individual commits provide a little more information, but overall point - do not configure logger unless it is an "end user application". And it is typical for logging to just go into stderr to not interfere with potential "useful" output on stdout.