Skip to content

bpo-29905: Fix TypeError in asyncio/proactor_events #993

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

Merged
merged 1 commit into from
Jun 10, 2017
Merged

bpo-29905: Fix TypeError in asyncio/proactor_events #993

merged 1 commit into from
Jun 10, 2017

Conversation

DimitrisJim
Copy link
Contributor

@DimitrisJim DimitrisJim commented Apr 4, 2017

A new PR for issue29905, without including the TypeError reformatting for asynchat which, if my interpretation of #issue25002 and specifically #msg250151 was correct, should probably not be included.

If my interpretation was incorrect, I'll push a change for it too :-).

Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That if branch doesn't seem to be tested in Lib/test/test_asyncio/test_proactor_events.py. Could you add a simple test that triggers TypeError? The ProactorSocketTransportTests.socket_transport() helper already provides most of the needed infrastructure so I guess it should be easy to write a test case.

Please add a note to Misc/NEWS.

Also, "is" in your PR title should be "in", right? :)

@@ -232,8 +232,9 @@ class _ProactorBaseWritePipeTransport(_ProactorBasePipeTransport,

def write(self, data):
if not isinstance(data, (bytes, bytearray, memoryview)):
raise TypeError('data argument must be byte-ish (%r)',
type(data))
msg = ("data argument must be a bytes-like object, not '%s'" %
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%r looks OK to me. Is there a reason to use '%s' with type(data).__name__?

Copy link
Contributor

@AraHaan AraHaan Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use string tuples for format like this?

            msg = ("data argument must be a bytes-like object, not '{}'"
            ).format(type(data).__name__)

And then it would eliminate any possible confusion on the %'s.

If you are wondering why I prefer string tuples. It is because of the fact that it helps simplify a lot of code or on the other hand helps make the code confine to PEP8.

For example lets say you have this code with these objects:

class Cat:
    """
    Class that makes up an instance of Cat.
    """
    def __init__(self, name, age, location):
        self.name = name
        self.age = age
        self.location = location
class Dog:
    """
    Class that makes up an instance of Dog.
    ""
    def __init__(self, name, age, location):
        self.name = name
        self.age = age
        self.location = location

Since both objects have the same attributes instead of doing an:

"My {0} is named {1} and is {2} years old and is at {3}".format(type(myobj).__name__, myobj.name, myobj.age, myobj.location)

it could become:

"My {type(0).__name__} is named {0.name} and is {0.age} years old and is at {0.location}".format(myobj)

As you can see simplifies things a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@berkerpeksag AFAIK repring in error messages is used to play it safe in cases where the object happens to be an instance of bytes; the __name__ special attribute is always a string so %s is always certain to work. Apart from that, I followed along with other cases of formatting __name__ I encountered in other error messages.

@AraHaan The author of the module prefers using old-style formatting, so I stuck with that to keep it consistent. :-)

@berkerpeksag berkerpeksag requested a review from 1st1 April 19, 2017 04:59
@DimitrisJim
Copy link
Contributor Author

I'll address the requests soon, thanks!

(Yes, silly typo :-))

@DimitrisJim DimitrisJim changed the title bpo-29905: Fix TypeError is asyncio/proactor_events bpo-29905: Fix TypeError in asyncio/proactor_events Apr 19, 2017
@1st1 1st1 merged commit 34792d2 into python:master Jun 10, 2017
1st1 pushed a commit that referenced this pull request Jun 10, 2017
1st1 pushed a commit that referenced this pull request Jun 10, 2017
@1st1
Copy link
Member

1st1 commented Jun 10, 2017

Thanks! Will be shipped in 3.6.2

1st1 added a commit that referenced this pull request Jun 10, 2017
1st1 added a commit that referenced this pull request Jun 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants