-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Merging logging-stdlib-handler-feature branch back into master #2151
Conversation
Doesn't make sense but let me quickly review |
Yeah bad commit that should be nuked |
@waprin Where was it from? Stray commit from bad rebase? Also why did CLA bot not yell at me? |
When you say nuke do mean folded into the original or just deleted? |
deleted |
54f3b94
to
1916cca
Compare
@waprin I deleted and in the process decided to fold in the rebase changes needed to avoid the weird extra commit. I made some other changes, but want them to be clear so it doesn't seem to nanny-ish (it is nanny-ish, but I think the changes are good?): https://gist.github.com/dhermes/02bf516901721cb3d2a3c48cd93958b0 As you can see the changes are the following:
We can address outside this PR but I'm also pretty uncomfortable with
|
@dhermes changes look good.
Why do you consider this a problem? I'm fine to change it, we could roll that code into
Just not sure what to do with this, if you want I can help with googleapis/oauth2client#549 if that will expedite anything. |
Refactors handlers into separate package Adds background threaded transport Adds fix to Batch commit to properly set log name
@waprin It's the repeated name, just feels wrong / like the base-package isn't named correctly. As for the copy, why is it there? |
The repeated name, I agree just struggling to think of better name. The whole thing is "add Python logging handler functionality", The copy is there because since it uses the client on a background thread to write the logs, there's no guarantee the main thread isn't using the client at the same time. There's no way to lock the client and the system tests were occasionally failing due to this, copying seems the most sane approach. |
|
Fixes #2102
Will likely fail the CLA check (since it contains someone else's commits) but we can just merge with a failed check.
/cc @waprin