search

Ape / samsungctl

Remote control Samsung televisions via a TCP/IP connection
MIT License
712 stars 189 forks source link

Logging fixes and improvements #103

Open scop opened 5 years ago

scop commented 5 years ago

See individual commit messages for more details.

Ape commented 5 years ago

I cherry picked the first commit, but I think I am not going to merge the other one.

scop commented 5 years ago

Can I ask why not?

For example, samsungctl is used within Home Assistant's samsungtv component. It logs in my opinion too verbosely, and because it's using the root logger, it's not trivial to even find out where the too verbose log messages originate from.

With the current setup, there's pretty much nothing that can sanely be done about either issue. Having the remaining commit of this PR applied would fix the logger identity issue, and empower users to tweak the logging as they see fit without affecting the entire Home Assistant setup, with no draw backs I'm aware of.

Olen commented 4 years ago

I agree with scop on this. What is the problem with using package specific logging? Please merge, as it makes filtering of logs in other apps much easier.

foreign-sub commented 4 years ago

Hi, as an other Home Assistant user, i have been wondering for some times why my log was filled with lots of useless data, a google search quickly helped me to identify the culprit only to discover a fix has been ready to commit for months but has never been merged. I would even go further and turn every single message to debug level. There is really no point being so verbose about the API activity if it just works as intended, please reconsider merging this PR. This will clean up hundreds of log files and in the meantime reduce global warming. You can help us save the planet ! Thank you for your assistance and creating this api.

connesc commented 4 years ago

I would also love to see this merged, for exactly the same reasons as above. BTW, thanks for your work @Ape! :+1:

kleinc80 commented 2 years ago

Hi there, Happy new Year everyone! I know the PR and conversation are quite old but I'll give it a try. :-) I'm also using this nice piece of work from @Ape in my HomeAssistant and for the reasons mentioned I'd love to see @scop's PR merged! Cheers!