Get rid of legacy class StreamWriter #2109 #2651

pfreixes · 2018-01-10T00:11:37Z

What do these changes do?

Legacy StreamWriter class as a pure proxy of the transport and the protocol is
no longer needed. All of the functionalities that were behind this class
has been moved to the PayloadWriter class.

Some changes that have to be considered that impacted by this change

TCP Operations have been isolated in a module rather than move them
into the PayloadWriter class. All of these operations have been simplified and state less.
WebSocketWriter class had a dependency with the StreamWriter, to get rid of
that dependency the constructor has been modified to take the protocol
and the transport.

A next step changing the name of the PayLoadWriter class for the StreamWriter - it reborn!- might be considered, making it consistent with the reader side.

Are there changes in behavior for the user?

No

Related issue number

#2109

Checklist

I think the code is well written
Unit tests for the changes exist
[] Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

Legacy StreamWriter as a pure proxy of the transport and the protocol is no longer needed. All of the functionalities that were behind this class has been moved to the PayloadWriter. Some changes that have to be considered that impacted during this change * TCP Operations have been isolated in a module rather than move them into the PayloadWriter * WebSocketWriter had a dependency with the StreamWriter, to get rid of that dependency the constructor has been modified to take the protocol and the transport. A next step changing the name PayLoadWriter for the StreamWriter to have consistency with the reader part, might be considered.

fafhrd91 · 2018-01-10T00:21:42Z

looks good to me

asvetlov · 2018-01-10T14:07:06Z

aiohttp/http_writer.py

        return self._transport

+    @property
+    def protocol(self):
+        return self._protocol


Please add a test for the property

asvetlov

PayLoadWriter -> StreamWriter renaming sounds reasonable too

codecov-io · 2018-01-10T15:45:48Z

Codecov Report

Merging #2651 into master will increase coverage by 0.06%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #2651      +/-   ##
==========================================
+ Coverage   97.92%   97.98%   +0.06%     
==========================================
  Files          38       39       +1     
  Lines        7272     7245      -27     
  Branches     1262     1257       -5     
==========================================
- Hits         7121     7099      -22     
+ Misses         47       45       -2     
+ Partials      104      101       -3

Impacted Files	Coverage Δ
aiohttp/http.py	`100% <ø> (ø)`	⬆️
aiohttp/client_proto.py	`94.57% <100%> (-0.09%)`	⬇️
aiohttp/http_websocket.py	`98.61% <100%> (ø)`	⬆️
aiohttp/http_writer.py	`100% <100%> (+0.75%)`	⬆️
aiohttp/web_protocol.py	`91.57% <100%> (+2.23%)`	⬆️
aiohttp/client_reqrep.py	`97.36% <100%> (ø)`	⬆️
aiohttp/web_fileresponse.py	`97.87% <50%> (-0.02%)`	⬇️
aiohttp/tcp_helpers.py	`89.28% <89.28%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74810c2...884a4cf. Read the comment docs.

asvetlov · 2018-01-10T15:50:32Z

Another question: why do you return a value from tcp_cork/tcp_nodelay but never use it in aiohttp code?
Let's drop this functionality.
Tests can check a real TCP option by getsockopt call instead of checking the return value.

pfreixes · 2018-01-10T21:23:43Z

@asvetlov removed the value returned by the TCP operations. My fault Ive ended up changing the code to have an easy way to test the functions, once these have become stateless there is some edge cases where I cant see how to assert properly. For example the case of the socket None.
Any idea how to improve this [1]?

[1] 87928be#diff-ef69e535e777102409aee677f44b4137R75

asvetlov · 2018-01-11T09:37:15Z

You can monkey-patch socket object to make sure that its methods are not called but I think current code is good enough.

asvetlov · 2018-01-11T09:37:30Z

Thanks!

lock · 2019-10-28T11:03:41Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

pfreixes changed the title ~~Get rid of legacy StreamWriter (#2623)~~ Get rid of legacy StreamWriter #2623 Jan 10, 2018

pfreixes mentioned this pull request Jan 10, 2018

do we need http pipelining? #2109

Closed

pfreixes changed the title ~~Get rid of legacy StreamWriter #2623~~ Get rid of legacy StreamWriter #2109 Jan 10, 2018

Add CHANGES

6316553

pfreixes changed the title ~~Get rid of legacy StreamWriter #2109~~ Get rid of legacy class StreamWriter #2109 Jan 10, 2018

pfreixes added 4 commits January 10, 2018 01:25

Merge branch 'master' into remove_streamwriter

050f36a

Fixed invalid import order

58d14a2

Fix test broken

95eba9c

Fix tcp_cork issues

87fddb1

asvetlov requested changes Jan 10, 2018

View reviewed changes

Test PayloadWriter properties

4d977fa

asvetlov approved these changes Jan 10, 2018

View reviewed changes

Avoid return useless values for tcp_<operations>

87928be

pfreixes added 3 commits January 10, 2018 22:34

Merge branch 'master' into remove_streamwriter

4a63159

Increase coverage http_writer

c9673ad

Increase coverage web_protocol

884a4cf

asvetlov merged commit f570fed into master Jan 11, 2018

asvetlov deleted the remove_streamwriter branch January 11, 2018 09:37

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of legacy class StreamWriter #2109 #2651

Get rid of legacy class StreamWriter #2109 #2651

pfreixes commented Jan 10, 2018 •

edited

Loading

fafhrd91 commented Jan 10, 2018

asvetlov Jan 10, 2018

pfreixes Jan 10, 2018

asvetlov left a comment

codecov-io commented Jan 10, 2018 •

edited

Loading

asvetlov commented Jan 10, 2018

pfreixes commented Jan 10, 2018

asvetlov commented Jan 11, 2018

asvetlov commented Jan 11, 2018

lock bot commented Oct 28, 2019

Get rid of legacy class StreamWriter #2109 #2651

Get rid of legacy class StreamWriter #2109 #2651

Conversation

pfreixes commented Jan 10, 2018 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

fafhrd91 commented Jan 10, 2018

asvetlov Jan 10, 2018

Choose a reason for hiding this comment

pfreixes Jan 10, 2018

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 10, 2018 • edited Loading

Codecov Report

asvetlov commented Jan 10, 2018

pfreixes commented Jan 10, 2018

asvetlov commented Jan 11, 2018

asvetlov commented Jan 11, 2018

lock bot commented Oct 28, 2019

pfreixes commented Jan 10, 2018 •

edited

Loading

codecov-io commented Jan 10, 2018 •

edited

Loading