Add LiveStream component and endpoint #72
Conversation
renefs
commented
Apr 14, 2020
renefs
commented
- Add endpoint for the live_stream and live_stream_status
- Fix patch method: Using data instead of params for kwargs
renefs
changed the title
|
Tox is reporting ok and the code formatting is ok as well. Once Travis conf is fixed, the checks should succeed :) |
|
Hi @renefs looks like there are a bunch of failing tests -- Could I trouble you to fix those up and then I'll review the code. Thanks! |
Updating the fork develop branch
|
Done, It seems since my last PR, the develop branch changed a bit, but now It should be ok |
There was a problem hiding this comment.
Hi @renefs . Thank you for putting this PR together!
We just switched over to using the responses library for testing purposes. Could I trouble you to merge the most recent develop into your branch and update your tests accordingly? I think you'll find that it's a better way to write tests and there will be fewer chances of sneaky bugs in the future.
Thank you again for your effort!
There was a problem hiding this comment.
Thanks for the quick update with the tests. I've added a question for you and my preference would be to combine the two LiveStream and LiveStreamUpdate components to one for the sake of simplicity in the python library, even if it's technically 2 separate endpoints on Zoom's side. What are your thoughts?
| @@ -3,4 +3,3 @@ repos: | |||
| rev: stable | |||
| hooks: | |||
| - id: black | |||
| language_version: python3.6 | |||
There was a problem hiding this comment.
Was there a reason this was removed?
There was a problem hiding this comment.
The precommit was failing for me when using Python 3.7. I think that removing this line, the Python version was not forced. Correct me if I'm wrong though.
There was a problem hiding this comment.
Ahh, got it. We'll want to keep this as it was before since that ensures we're all running it using the same python version. If this is causing you issues on the commit side of things, you could temporarily remove this and run black manually to make sure that files are formatted correctly. Then you can add it back in so that it works for everyone else. Thoughts?
There was a problem hiding this comment.
I'm afraid that I cannot commit if I remove this line
|
Apply the changes as you wish |
|
@renefs FYI, I just went ahead and fixed up the merge conflicts. Can you double check if anything else seems amiss and respond to any other questions not pertaining to the merge part and then we can get this merged in today. Thanks for your time and effort! |
|
Hi @prschmid! May I bump this PR up for merging? The livestream feature is something that I would like to utilize for my research project. Thank you! |
There was a problem hiding this comment.
Checked out locally and tests passed. Code looks good and will definitely be useful! I think we are ready to merge but I'd like @prschmid to take a final pass before we merge.
