Avoid use of root logger and fix close callback in python3 - #1415
Merged
Conversation
- Using the root logger makes it non-obvious for users of the library
to control the log messages from the library.
- By setting up a logger for ola and using it for all the logging
calls, users of the library can simply do logging.getlogger('ola')
and control the output of the logger.
- However, even though the convention for library loggers is to
attach a do nothing handler and not propagate messages, it is not
followed here because the library logged directly to the
root logger before, and removing all log messages will then
cause missing output for users that expected log entries.
- Instead, the configuration for the library's logger emulates what
basicConfig() would do - setup a StreamHandler and a default formatter
that uses BASIC_FORMAT, and outputs logs in this configuration, to
emulate past behaviour as closely as possible, for users that
never configured the root logger.
- There are two issues, though. If the user did not setup the
root logger and OLA emits log messages, they will now appear as
before, but without the designation that they were
from the root logger, but designated as coming from the logger
named ola.
- If the user of the library knew that OLA was emitting messages from
the root logger and altered the behaviour of the root logger to
alter log emission, they will find that it is useless, as OLA
now by default logs straight to standard error through the
StreamHandler. To change OLA's logging behaviour, they will have to
remove the default StreamHandler added and alter propagation settings.
Contributor
Author
|
|
Member
|
Yeah go for it @shenghaoyang . Just retitle the PR to suit. |
- In python3, recv() no longer returns a string object, so comparing against an empty string will always fail and the close callback is never called - program never gets notified that the socket is closed. - Both b'' and '' evaluate to false, so just check if the recv() call returns an object that is implicitly false and invoke the callback accordingly.
shenghaoyang
changed the title
peternewman
requested changes
May 11, 2018
|
|
||
| import logging | ||
|
|
||
| """Initialization code for the ola python library""" |
Member
There was a problem hiding this comment.
Codacy doesn't like this:
"String statement has no effect"
https://app.codacy.com/app/peternewman/ola/pullRequest?prid=1616358
I think it may need to be above the import, looking at some other real world examples.
Contributor
Author
|
Yeah, my apologies, I messed up the docstring. It's supposed to be the
first actual statement. I'll get a fixup in tomorrow - was wondering why
codacy wasn't happy.
…On Fri, 11 May 2018, 22:32 Peter Newman, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In python/ola/__init__.py
<#1415 (comment)>
:
> +#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street <https://maps.google.com/?q=51+Franklin+Street&entry=gmail&source=g>, Fifth Floor, Boston, MA 02110-1301 USA
+#
+# __init__.py
+# Copyright (C) 2018 Shenghao Yang
+
+import logging
+
+"""Initialization code for the ola python library"""
Codacy doesn't like this:
"String statement has no effect"
https://app.codacy.com/app/peternewman/ola/pullRequest?prid=1616358
I think it may need to be above the import, looking at some other real
world examples.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1415 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJ5CDB7CTJkJynY8EthuWSH4VmtzVsW8ks5txaEMgaJpZM4Tzqos>
.
|
peternewman
approved these changes
Jun 9, 2018
peternewman
left a comment
peternewman
left a comment
Member
There was a problem hiding this comment.
LGTM thanks @shenghaoyang !
nomis52
approved these changes
Jun 9, 2018
Draft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Using the root logger makes it non-obvious for users of the library
to control the log messages from the library.
By setting up a logger for ola and using it for all the logging
calls, users of the library can simply do logging.getLogger('ola')
and control the output of the logger.
Note that any tools / tests depending on log output might need to
be changed due to the reasons above. Those changes can be done,
but I want to put this commit out first so we can discuss this approach,
or we could skip emulating past behavior and simply adopt
the convention for python libraries.
Empty string object is no longer returned by
recv()in python3on connection close, but an empty bytes object is returned instead.
Comparing the return value to an empty string is insufficient.
Since both objects evaluate to false, check for that condition and
invoke close actions accordingly.