Skip to content
This repository has been archived by the owner on Jan 13, 2021. It is now read-only.

Fxied to call _send_cb() instead of _data_cb() for PingFrame #79

Merged
merged 2 commits into from
Jan 24, 2015

Conversation

t2y
Copy link
Contributor

@t2y t2y commented Jan 24, 2015

HTTP20Connection object doesn't have _data_cb() method. I guess it's a typo? Using _send_cb() works well.

(hyper)$ python
Python 3.4.2 (default, Nov 23 2014, 23:10:23) 
[GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.54)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from hyper import HTTP20Connection
>>> conn = HTTP20Connection('google.com')
>>> conn.request('GET', '/')
1
>>> conn.getresponse()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../hyper/http20/connection.py", line 171, in getresponse
    return HTTP20Response(stream.getheaders(), stream)
  File ".../hyper/http20/stream.py", line 293, in getheaders
    self._recv_cb()
  File ".../hyper/http20/connection.py", line 541, in _recv_cb
    self._consume_single_frame()
  File ".../hyper/http20/connection.py", line 521, in _consume_single_frame
    self.receive_frame(frame)
  File ".../hyper/http20/connection.py", line 342, in receive_frame
    self._data_cb(p, True)
AttributeError: 'HTTP20Connection' object has no attribute '_data_cb'

@t2y
Copy link
Contributor Author

t2y commented Jan 24, 2015

hmm, this test indicate to add a function as _data_cb. Is this proper usage?

    def test_ping_without_ack_gets_reply(self):
        c = HTTP20Connection('www.google.com')
        f = PingFrame(0)
        f.opaque_data = b'12345678'

        frames = []

        def data_cb(frame, tolerate_peer_gone=False):
            frames.append(frame)
        c._data_cb = data_cb
>       c.receive_frame(f)

@Lukasa
Copy link
Member

Lukasa commented Jan 24, 2015

Thanks for this @t2y!

Good spot, this is a typo. Correctly, you've also spotted that test_ping_without_ack_gets_reply contains the same error. Additionally, test_ping_with_ack_ignored also contains that error. Looks like I was having a bad day when I wrote the PING frame code.

If you want to fix both those tests and add yourself to AUTHORS.rst, I'll happily merge this. =) 🍰

@t2y
Copy link
Contributor Author

t2y commented Jan 24, 2015

If you want to fix both those tests and add yourself to AUTHORS.rst,

Thanks. I just fixed a simple typo. Now I'm learning http2 with hyper and want to contribute something, then I'll add myself 😄

@Lukasa
Copy link
Member

Lukasa commented Jan 24, 2015

Fair enough @t2y. =)

You got a bit over-eager on the tests, though. They were both testing the correct thing, I just wrote them incorrectly. It should be enough to just change the line c._data_cb = data_cb to read c._send_cb = data_cb. If you do that and the tests fail, I would be extremely interested to know that.

@t2y
Copy link
Contributor Author

t2y commented Jan 24, 2015

Thanks. I misunderstood and fixed correctly. 😅

@Lukasa
Copy link
Member

Lukasa commented Jan 24, 2015

No problem at all! This is perfect, thank you so much @t2y! 🍰 ⭐ 👍

Lukasa added a commit that referenced this pull request Jan 24, 2015
Fxied to call _send_cb() instead of _data_cb() for PingFrame

Thanks to @t2y!
@Lukasa Lukasa merged commit 7978b2a into python-hyper:development Jan 24, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants