-
Notifications
You must be signed in to change notification settings - Fork 35
Add logging control #1585
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
Add logging control #1585
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1585 +/- ##
==========================================
- Coverage 95.33% 93.00% -2.34%
==========================================
Files 183 184 +1
Lines 15770 16033 +263
==========================================
- Hits 15035 14911 -124
- Misses 735 1122 +387
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
pre-commit.ci autofix |
fff0ad6 to
d01b937
Compare
|
pre-commit.ci autofix |
|
didn't fix the build: I'll investigate this more once I get the FAQ doc finished |
|
Not sure if we need a news entry or not |
IAlibay
left a comment
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.
This looks good to me, but please let @atravitz have a look before merging.
|
Do we want to prefix these classes with a |
I'm in favor of a news entry that points to the new docs, since people have been raising this as a bug and might look for it in a CHANGELOG. |
Co-authored-by: Alyssa Travitz <31974495+atravitz@users.noreply.github.com>
|
hmm, @atravitz ruff wants to change this: Maybe b/c its relative it is being weird, but I would rather have these on separate lines or so that the diff will be clean if we want to add more (or remove one) |
|
It looks like it was happy when I manually did it in this style: not sure if we can automate that or not |
|
need to add the news bit still |
I don't have a super strong opinion about this, the way you have it here is good though. |
I like the idea of having it user facing at some point (could be helpful for colab headaches?). But let's make them private for now - we can always make them public in a follow-up PR. |
|
No API break detected ✅ |
|
@atravitz Sounds good! Renamed everything |
atravitz
left a comment
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.
looks great!

Checklist
newsentryDevelopers certificate of origin
Fixes #1499