♻️ Delete size arg from StreamReader.feed_data
#7265
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
for more information, see https://pre-commit.ci
Co-authored-by: Sam Bull <aa6bs0@sambull.org>
|
It wouldn't hurt to write actual full commit messages and PR titles. When I get a notification about "something happened with todo" or "some improvement", those don't look important enough to compell me not to skip the notifications. I have hundreds of those and will choose things that look meaningful and related to what I care about. |
DavidRomanovizc
changed the title
webknjaz
changed the title
size parameter from StreamReader.feed_data
webknjaz
changed the title
size parameter from StreamReader.feed_datasize arg from StreamReader.feed_data
Co-authored-by: Sam Bull <aa6bs0@sambull.org>
|
CI also crashes due to the missing of the |
|
Error seems pretty clear: Method is defined at: https://github.com/DavidRomanovizc/aiohttp/blob/master/aiohttp/http_parser.py#L854 |
|
Presumably, the same changes can be made, using |
DavidRomanovizc
changed the title
size arg from StreamReader.feed_datasize arg from StreamReader.feed_data
Codecov Report
@@ Coverage Diff @@
## master #7265 +/- ##
==========================================
- Coverage 97.30% 97.29% -0.02%
==========================================
Files 106 107 +1
Lines 31456 31481 +25
Branches 3940 3928 -12
==========================================
+ Hits 30608 30629 +21
- Misses 644 647 +3
- Partials 204 205 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 15 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Co-authored-by: Sam Bull <aa6bs0@sambull.org>
for more information, see https://pre-commit.ci
| @@ -580,9 +579,8 @@ def set_exception(self, exc: BaseException) -> None: | |||
| self._waiter = None | |||
| set_exception(waiter, exc) | |||
|
|
|||
| def feed_data(self, data: _T, size: int = 0) -> None: | |||
| self._size += size | |||
There was a problem hiding this comment.
Why is this removed? This will surely break the limit functionality. Just use len(data) to calculate the actual size?
There was a problem hiding this comment.
And if we don't delete this line, the TestFlowControlDataQueue.test_resume_on_read test will fall
| @@ -608,8 +606,7 @@ async def read(self) -> _T: | |||
| raise | |||
|
|
|||
| if self._buffer: | |||
| data, size = self._buffer.popleft() | |||
| self._size -= size | |||
There was a problem hiding this comment.
Because if we don't delete it we get error in almost all tests the TypeError error: unsupported operand type(s) for -=: 'int' and 'StreamReader'. In tests/test_client_functional.py and he refers to this block of code
There was a problem hiding this comment.
len(data)? Obviously you can't subtract a StreamReader from an int...
Though if it's possible for data to be something other than bytes, then maybe this won't work.
There was a problem hiding this comment.
OK, but we will still have tests falling in TestDataQueue. With TypeError error: object of type 'object' has no len(). Now it seems that we need to use b"something" instead of object() or handle error, but I'm not sure about that.
There was a problem hiding this comment.
Well, object() is probably unreasonable, so we could maybe update that test... I'll have a look through later.
There was a problem hiding this comment.
I added types checking to feed_data and now those tests don't crash, but I doubt the correctness of such a solution
What do these changes do?
Removed the unused
sizeparameter from thefeed_datamethod to simplify the code and usage of the method.Are there changes in behavior for the user?
If the user is using the feed_data method without the size parameter, there should be no changes in the behavior of the method for them. However, if the size parameter was used, there may be changes in the behavior of the method.
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.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.