-
Notifications
You must be signed in to change notification settings - Fork 16
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
24.toggle debugging #25
base: master
Are you sure you want to change the base?
24.toggle debugging #25
Conversation
@@ -131,6 +123,22 @@ def __getattr__(self, name): | |||
else: | |||
return val | |||
|
|||
def _logging_callback(self, level, domain, message, data): |
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.
Does this have to be an instance method?
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.
No it doesn't. It's an artifact of older code.
Originally I had the if self.logging_enabled
check in there, but then I moved it up to the wrapper in __init__()
. We have three options:
- Leave it be and merge the pull request. No harm done
- Move the
if self.logging_enabled
back to the_logging_callback()
function. This would make it a little more clean if we wanted to add some other features toLibraryWrapper
that effect logging - Move the body of
_logging_callback()
to_logging_callback_wrapper()
in__init__()
.
Which one do you want?
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.
Have you tried getting rid of the inner wrapper function with 2)? I'm not sure how cffi handles callbacks that are bound instance methods, but that would probably the cleanest way from a readability perspective. Otherwise I'd probably go with 3) :-)
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.
Instance methods didn't worked as I hoped that they would. I'll make the modifications for No. 3 and update the pull request.
After some testing I think this solution might have been incorrect. I swear it was working last night where it would let me toggle logging in my application. Could you try pulling down this branch and see if it works for you? |
I'm thinking that there might have been some threading issue with this. |
66b1c10
to
7211985
Compare
Addressing #24, the programmer can now toggle logging on and off. I did this without using statics. Instead I created a wrapper function (inside of
__init__
) that will check ifself.enable_logging
isTrue
orFalse
. And dependent upon that, it will actually log the message.Also it bumps up the version number to 0.4.2. If this looks good, please do a push to PyPi.