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

Support partial request for media resource #1050

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@MarschHuynh
Contributor

MarschHuynh commented Aug 20, 2017

I am creating an API for record and playback audio. I use Eve as a main lib for my project.
Without partial request, my player in my phone (iPhone) can not work.

So I change a little code to make Eve support partial request for the media resource.

Hope that makes sense.

Thanks.

@Amedeo91

This comment has been minimized.

Amedeo91 commented Aug 22, 2017

@nicolaiarocci: what do you think? Personally I do not see test coverage...

@nicolaiarocci

This comment has been minimized.

Member

nicolaiarocci commented Aug 30, 2017

Hello and thanks for this. It is indeed a very cool feature. However, it would also need test coverage.

if g[0]:
byte1 = int(g[0])
if g[1]:
byte2 = int(g[1])

This comment has been minimized.

@peterdemin

peterdemin Aug 30, 2017

g[1] might be empty string, which will break int(''):

>>> int('')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: invalid literal for int() with base 10: ''

This comment has been minimized.

@MarschHuynh

MarschHuynh Aug 30, 2017

Contributor

@nicolaiarocci Yes, I will update the test as soon as possible.

@peterdemin Thank you for correcting me.

@nicolaiarocci

This comment has been minimized.

Member

nicolaiarocci commented Nov 7, 2017

This is still waiting on proper test coverage. Are you still planning to add it?

@nicolaiarocci nicolaiarocci added this to the 0.8 milestone Nov 7, 2017

@nicolaiarocci

This comment has been minimized.

Member

nicolaiarocci commented Dec 26, 2017

Thanks for the commit update. I really want to merge this but I can't in its current state. Still waiting for test coverage.

nicolaiarocci added a commit that referenced this pull request Apr 5, 2018

nicolaiarocci added a commit that referenced this pull request Apr 5, 2018

nicolaiarocci added a commit that referenced this pull request Apr 5, 2018

nicolaiarocci added a commit that referenced this pull request Apr 5, 2018

nicolaiarocci added a commit that referenced this pull request Apr 5, 2018

nicolaiarocci added a commit that referenced this pull request Apr 5, 2018

nicolaiarocci added a commit that referenced this pull request Apr 5, 2018

nicolaiarocci added a commit that referenced this pull request Apr 5, 2018

@nicolaiarocci

This comment has been minimized.

Member

nicolaiarocci commented Apr 5, 2018

Refactored, rebased, test-covered, and finally merged. See 9ee2b5b

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