-
-
Notifications
You must be signed in to change notification settings - Fork 647
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
Port majority of pants/util #6073
Changes from 6 commits
cf97ca8
e1daf89
468627c
323b7d0
6eaf573
10a3a88
b5358ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
|
||
import logging | ||
import time | ||
from builtins import range | ||
|
||
|
||
logger = logging.getLogger(__name__) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
unicode_literals, with_statement) | ||
|
||
import threading | ||
from builtins import bytes, object | ||
|
||
from six import StringIO | ||
|
||
|
@@ -34,8 +35,10 @@ def read_from(self, pos, size=-1): | |
return self._io.read() if size == -1 else self._io.read(size) | ||
|
||
def write(self, s): | ||
if not isinstance(s, bytes): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @benjyw follow up from prior discussion. Good point about prior version changing semantics. This does change semantics that now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, but that coercion might have failed, so it's best to at least fail predictably and eagerly. |
||
raise ValueError('Argument not in bytes: {0}'.format(s)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be more useful to say what type the argument is, e.g., Side note - we typically don't enumerate the format args, unless we're repeating them (in which case we prefer to give them names instead of ordinals). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good on both. Can't wait for us to be able to use f-strings! Thanks for the review |
||
with self._lock: | ||
self.do_write(str(s)) | ||
self.do_write(s) | ||
self._io.flush() | ||
|
||
def flush(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
import io | ||
import select | ||
import socket | ||
from builtins import object | ||
|
||
|
||
def teardown_socket(s): | ||
|
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.
Temporarily removing the override of
str()
because it breaks a bunch of tests. I'll figure out how to fix this in a separate dedicated PRThere 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.
Actually for now I'm going keep the original semantics. I don't understand the ramifications to changing this.
We might run into issues when running Python3 tests the first time - if so, we'll know we have to change the semantics from what we have now.
Below change (line 159) makes it so the semantics is the same in Py2 and Py3.