Skip to content

Conversation

@V3ntus
Copy link
Contributor

@V3ntus V3ntus commented Jan 13, 2022

About

Related to #421

Unless a simple logging configuration is setup, log messages should be hidden. Users can set the level with logging.basicConfig(level=logging.DEBUG) ex. if they desire. interactions should then propagate its loggers to that StreamHandler

A list of loggers (their names) is present in base.Data which gets populated after importing interactions

Checklist

  • I've ran pre-commit to format and lint the change(s) made.
  • I've checked to make sure the change(s) work on 3.8.6 and higher.
  • This fixes/solves an Issue.
    • (If existent):
  • I've made this pull request for/as: (check all that apply)
    • Documentation
    • Breaking change
    • New feature/enhancement
    • Bugfix

@Catalyst4222
Copy link
Contributor

Personally, I think that Warning, Error, and Critical should be shown if it directly relates to the user

So (as examples) a duplicate command should warning/error to stderr, but the critical from missing attributes shouldn't be shown

@V3ntus
Copy link
Contributor Author

V3ntus commented Jan 13, 2022

Personally, I think that Warning, Error, and Critical should be shown if it directly relates to the user

I agree. I suppose NullHandlers would be a bad idea as default then. The missing attribute warning spam gets somewhat irritating, and confusing for some as well. Maybe we can pass errors and critical?

@V3ntus V3ntus changed the title Fix logging: default to NullHandler Minor logging refactor Jan 13, 2022
@V3ntus
Copy link
Contributor Author

V3ntus commented Jan 13, 2022

e500293 should allow errors, but still give users the ability to define a logging.basicConfig with a level of their choice. This also should prevent duplicate log messages in console.

Tested

Copy link
Contributor

@EepyElvyra EepyElvyra left a comment

Choose a reason for hiding this comment

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

Except those two smaller issues this looks fine to me

@V3ntus
Copy link
Contributor Author

V3ntus commented Jan 15, 2022

Except those two smaller issues this looks fine to me

Thanks, missed those two

@EepyElvyra
Copy link
Contributor

Except those two smaller issues this looks fine to me

Thanks, missed those two

no problem

Copy link
Contributor

@EepyElvyra EepyElvyra left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@i0bs
Copy link
Contributor

i0bs commented Jan 20, 2022

I'm willing to include this as a refactor instead of a feature commit, but there's a merge conflict stopping me from doing that.

@i0bs i0bs changed the title Minor logging refactor refactor: Logger parse structure Jan 20, 2022
@V3ntus
Copy link
Contributor Author

V3ntus commented Jan 20, 2022

Alright I'll try to get that resolved today

@V3ntus
Copy link
Contributor Author

V3ntus commented Jan 20, 2022

Merged unstable, I don't see merge conflicts anymore, but let me know if there's still something wrong

@i0bs
Copy link
Contributor

i0bs commented Jan 24, 2022

Would recommend running pre-commit again, the checks have failed.

@i0bs
Copy link
Contributor

i0bs commented Jan 24, 2022

This PR LGTM. Thank you for contributing to the library for the first time, and welcome to the contributors club. 😃

@i0bs i0bs merged commit f0e4d77 into interactions-py:unstable Jan 24, 2022
@V3ntus V3ntus deleted the fix-logging-v4 branch January 24, 2022 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants