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

Fix: stream end when length of last chunk equal to MAX_CHUNK #356

Merged
merged 1 commit into from
Nov 8, 2017
Merged

Fix: stream end when length of last chunk equal to MAX_CHUNK #356

merged 1 commit into from
Nov 8, 2017

Conversation

PrimozGodec
Copy link
Contributor

@PrimozGodec PrimozGodec commented Oct 26, 2017

This pull request is fixing bug #355

I changed the approach of taking chunks form the generator, such that we know when we reach the last chunk with memorising the next chunk.

Copy link
Member

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Yup, this looks like it’ll work. Mind adding an entry to the change log and also a test for me?

@PrimozGodec
Copy link
Contributor Author

@Lukasa I changed the code and made unit test pass on my machine. I also added a test to test specific change.
Sorry I can not find a change log. Can you tell me where is it?
Also travis tests are failing it seems that they are failing because of some older code and not because of my changes.

@matjazp
Copy link
Contributor

matjazp commented Nov 2, 2017

@PrimozGodec, CI is fixed now, can you rebase please?

Also, change log is in HISTORY.rst and CONTRIBUTORS.rst.

@PrimozGodec
Copy link
Contributor Author

Thank you @matjazp. It is fixed now @Lukasa

self._send_chunk(chunk, final)
# since we need to know when we have a last package we need to know
# if there is another package in advance
cur_chunk = next(chunks)
Copy link
Member

Choose a reason for hiding this comment

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

This will explode if there are no chunks to send: I think you need to write a test case for that and refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! ✨

@Lukasa Lukasa merged commit a8109c3 into python-hyper:development Nov 8, 2017
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.

3 participants