Skip to content
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

bugfix: send() method; 'retry' parameter didn't work as specify #21

Merged
merged 2 commits into from
Apr 14, 2016

Conversation

ferboz
Copy link

@ferboz ferboz commented Apr 1, 2016

Previously 'retry' parameter match the absolute number of resend before failing.
With the fix 'retry' match the maximum number of resend a failed packet before failing

… documentation

Previously 'retry' parameter match the absolute number of resend before failing.
With the fix 'retry' match the maximum number of resend a failed packet before failing
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 63.344% when pulling 437d4f8 on ferboz:master into 695aa83 on tehmaze:master.

@ferboz
Copy link
Author

ferboz commented Apr 4, 2016

Could you guy help me in release this bug fix?
Thank you so much 👍

@ferboz ferboz changed the title Bugfix for send() method; 'retry' parameter didn't work as specify bugfix: send() method; 'retry' parameter didn't work as specify Apr 4, 2016
@krishardy
Copy link
Collaborator

No worries. Let me take a look. It make take a day or two.

Thanks!

@ferboz
Copy link
Author

ferboz commented Apr 6, 2016

Thank you Kris :)

@jquast
Copy link
Collaborator

jquast commented Apr 6, 2016

I approve this change, makes sense, for large data transfers we can expect some errors now and then, but they should not count against total transfer.

Would be nice if we unit test, but this change is simple enough to release without if too much trouble.

@ferboz
Copy link
Author

ferboz commented Apr 7, 2016

Jeff you have a fair point.
I will create unit test for it to ensure feature will not broken in the future.
I will propose a change in within 2 days.

@jquast
Copy link
Collaborator

jquast commented Apr 7, 2016

Thank you and good luck, I suspect the unit test will be harder than the change :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 63.344% when pulling ac8f77a on ferboz:master into 695aa83 on tehmaze:master.

@ferboz ferboz force-pushed the master branch 3 times, most recently from b85b6a3 to c1418c0 Compare April 8, 2016 11:11
@coveralls
Copy link

Coverage Status

Coverage increased (+4.0%) to 67.203% when pulling c1418c0 on ferboz:master into 695aa83 on tehmaze:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+4.0%) to 67.203% when pulling c1418c0 on ferboz:master into 695aa83 on tehmaze:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+4.3%) to 67.524% when pulling 1e68557 on ferboz:master into 695aa83 on tehmaze:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+4.3%) to 67.524% when pulling dc00598 on ferboz:master into 695aa83 on tehmaze:master.

@ferboz
Copy link
Author

ferboz commented Apr 8, 2016

Added few unit test for the retry parameter testing the send() method.
Please have a look and let me know if is ok for you :)

@jquast
Copy link
Collaborator

jquast commented Apr 13, 2016

Looks ok, I'll normalize the style, add a changelog, and push out a new build soon. Thanks for making the effort to add unit tests, and special thanks for your contribution, @ferboz !

@ferboz
Copy link
Author

ferboz commented Apr 13, 2016

Thank you @jquast to maintain open source!

@jquast jquast merged commit dc00598 into tehmaze:master Apr 14, 2016
@jquast
Copy link
Collaborator

jquast commented Apr 14, 2016

This is released to pypi as version 0.4.4.

By use of your new unit tests, I adjusted the boundaries of your max_resend variable to discover another bug included in PR #22, so it was very helpful, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants