Skip to content

Conversation

@timmarkhuff
Copy link

@timmarkhuff timmarkhuff commented May 5, 2025

I noticed while using this library that it makes a call to logging.basicConfig. I don't believe libraries should do this. logging.basicConfig should be called only once per runtime. Our software is trying to use python-ws-discovery, and we are finding that it is overriding our logging config.

@timmarkhuff
Copy link
Author

@andreikop , do you have any thoughts on this?

@andreikop
Copy link
Owner

@timmarkhuff, very good point.
Make a PR. I'll accept and release

@timmarkhuff
Copy link
Author

@timmarkhuff, very good point.

Make a PR. I'll accept and release

This is the PR. Is it sufficient?

@andreikop
Copy link
Owner

Ah, sorry, it was already PR and not a discussion.
Taking deeper look.
discover and publish are both command line entrypoints and setting up logging here is absolutely relevant.
The issue - the same functions are cmdline entrypoints and actually do the job.
I think basicConfig() shall remain at both those functions, but the code shall be refactored to move actual implementation to public API module. So, your application would not call wsdiscovery.cmdline.discover but wsdiscovery.discover.

wsdiscovery.discover shall be implemented in wsdiscovery.api and imported to both wsdiscovery top level module and to wsdiscovery.cmdline entrypoint. The same for publish API.

@timmarkhuff does it make sence? Could you do this refactoring?

@timmarkhuff
Copy link
Author

After looking at this more closely, I don't think the problem is with python-ws-discovery at all. The call to logging.basicConfig is in the cmdline.py file which isn't being called by our library. And as you said, it makes sense to have it in that file anyway.

I think the problem is with python-onvif that you use. I submitted a PR there, but there has been no activity on that repo for 5 years, so I'm not sure if anyone will see it.

@andreikop
Copy link
Owner

OK, closing this PR. But moving code from cmdline functions seems relevant. New PR is welcome.

@andreikop andreikop closed this Sep 30, 2025
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.

2 participants