fix: LoggingProxy.write() return type
#6791
Conversation
- The API of `IO.write()` is to return `int` corresponding to the length of the message - If we're substituting this class for `sys.stdout` it needs to follow the same interface
- The caller may intend to print whitespace to stdout - If they don't want this whitespace then ideally the calling method should control this rather than `LoggingProxy` mutating the message
076ff25 to
51f5b01
Compare
Codecov Report
@@ Coverage Diff @@
## master #6791 +/- ##
=======================================
Coverage 70.63% 70.64%
=======================================
Files 138 138
Lines 16594 16598 +4
Branches 2089 2090 +1
=======================================
+ Hits 11722 11726 +4
Misses 4677 4677
Partials 195 195
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I want to know more thought on this |
* fix: `LoggingProxy.write()` return type - The API of `IO.write()` is to return `int` corresponding to the length of the message - If we're substituting this class for `sys.stdout` it needs to follow the same interface * Don't mutate data to log - The caller may intend to print whitespace to stdout - If they don't want this whitespace then ideally the calling method should control this rather than `LoggingProxy` mutating the message
| @@ -223,7 +223,6 @@ def write(self, data): | |||
| if getattr(self._thread, 'recurse_protection', False): | |||
| # Logger is logging back to this file, so stop recursing. | |||
| return 0 | |||
| data = data.strip() | |||
| if data and not self.closed: | |||
There was a problem hiding this comment.
@ruaridhw: The strip() used to also remove newlines added by the print() function. With this fix, output of print() redirected to the logger will result in two newlines, instead of just one.
There was a problem hiding this comment.
Note: Before submitting this pull request, please review our contributing
guidelines.
Description
Updates the return type of
LoggingProxy.write()such that it conforms toIO.write().Additionally, removes the mutation of written data such as stripping whitespace. That said, this is quite a subjective change and was clearly added for a reason in the first place... It just feels odd to me that if
LoggingProxy's only job is to forward streams that it would also mutate them along the way.Fixes #6790