-
Notifications
You must be signed in to change notification settings - Fork 947
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
move logging configuration behind function call #1120
Conversation
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.
Small but important changes. I like the principle of the PR.
You need to add documentation of the call at highlevel in doc/source/library. |
I personally prefer to keep functionality cleanly separated into different files, which is why I prefer the private file with |
I have no problem having it a file ! but the file contains a public function so naming it _logging is at least confusing. If you just rename the file (and the function to keep the naming standard) I am happy. |
Please solve the pylint/flake8 problems. |
Happy to write docs, let me know more details please. You need me to edit doc/source/library/pymodbus.rst? |
Yeah I think that would be a good place to mention this function. Please write something like:
Do not use my words but the idea :-) I am a hard core programmer and have to torture myself to write documentation. Thanks for taking care of this. |
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.
Apart from the doc, this PR is good to be merged.
OK I added a docstring which should be rendered in autodoc and put mention of the function in a few places that made sense to me. Let me know what you think and if I should add more elsewhere. |
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.
All good apart from the pylint/flake8 error.
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.
LGTM
After this change users would need to call
pymodbus.apply_pymodbus_logging_config()
to enable logging as before. This way logs are not injected into my application in an unexpected way.Happy to change if maintainers wish, or feel free to reject if another option is prefered. A change to this behavior would be really great as my application has carefully formatted logs that will not be parsed correctly due to this injection.
fixes #1119