-
Notifications
You must be signed in to change notification settings - Fork 64
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
Library interferes with application logging configuration #23
Milestone
Comments
Could you do it in a PR?
…On Fri, Nov 20, 2020, 01:41 Dustin Oprea ***@***.***> wrote:
Because of this:
https://github.com/mediacloud/ultimate-sitemap-parser/blob/68d1ccdd573be16cdfadc7f071c088642182bff1/usp/fetch_parse.py#L43
and this:
https://github.com/mediacloud/ultimate-sitemap-parser/blob/68d1ccdd573be16cdfadc7f071c088642182bff1/usp/log.py#L36-L43
this library almost always ends-up reconfiguring the logging of the
calling application. Because it's running as soon as the module is
imported, it's configuring the logging before the host application has a
chance to. That aside, a package/library should just be using the logging,
not configuring the logging. That's the responsible of the front-end
scripts/application. It's disruptive, otherwise. It's usually not
appropriate to unconditionally print logging to the screen.
Can you please remove the code (shown above) where it's adding handlers to
the current logging config if no handlers already exists?
Also, this module is overriding all of the built-in logging functionality
unnecessarily. It could just use the logging object directly (e.g. _LOGGING
= logging.getLogger(__name__)), but that's just a side comment.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#23>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABMNPKHPULO2DGHQ72II73SQWUJHANCNFSM4T4CCXLA>
.
|
Sure. I just wanted to see if you'd respond, first. |
Bump |
1 similar comment
Bump |
A bit busy this week, I'll try to get to it soon though.
…On Mon, Nov 23, 2020 at 4:56 AM Dustin Oprea ***@***.***> wrote:
Bump
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#23 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABMNPN36KVPEJV5DWEVRSTSRHFOTANCNFSM4T4CCXLA>
.
--
Linas Valiukas
www.mediacloud.org
|
I was busy too, yet I made time for an extremely simple PR. |
Bump. |
2 similar comments
Bump. |
Bump. |
Hey @pypt - We need this change in our app too. Do you have a rough idea of when the change will be added? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Because of this:
https://github.com/mediacloud/ultimate-sitemap-parser/blob/68d1ccdd573be16cdfadc7f071c088642182bff1/usp/fetch_parse.py#L43
and this:
https://github.com/mediacloud/ultimate-sitemap-parser/blob/68d1ccdd573be16cdfadc7f071c088642182bff1/usp/log.py#L36-L43
this library almost always ends-up configuring the logging of the calling application in ways not often desired. It's usually not appropriate to unconditionally print logging to the screen. Can you please remove the code (shown above) where it's adding handlers if no handlers already exists?
Also, this module is overriding all of the built-in logging functionality unnecessarily. It could just use the logging object directly (e.g.
_LOGGING = logging.getLogger(__name__)
), but that's just a side comment.I'm currently having to do this to squash your output:
The text was updated successfully, but these errors were encountered: