Implementing Zoom APIv2 #11
Conversation
Update requirements.txt
|
As I continue down the rabbit hole of this PR, seeing how many conditional statements there are checking the version, I really want to refactor this to have an |
|
I decided to take a few hours while I was here at PyCon to finish up this PR. Hopefully it looks good! Please be sure to take a look at the issues I created, especially #12, in conjunction with this PR. |
|
@prschmid @imreACTmd Any chance y'all will be able to take a look at this, along with my comments? |
|
@tarkatronic sorry for the delay and thanks for the reminder! i've been swamped with other projects recently, but will take a look at it this week. thanks for all of your effort! |
| ```python | ||
| from zoomus import ZoomClient | ||
|
|
||
| client = ZoomClient('API_KEY', 'API_SECRET', version=2) |
There was a problem hiding this comment.
Should we make V2 the default? Yes this may break some scripts that people have, but V1 has been deprecated and most people should have switched over?
There was a problem hiding this comment.
I think a breaking change like that should require a major version bump. But, given that this is currently a pre-1.0 product, I think that would be acceptable, saying that the publicly defined API is V2 by default.
But I also think that in the future, removing Py2 support should require a major version bump. So it comes down to if you want that in 1.0.0 or 2.0.0.
There was a problem hiding this comment.
Let's make V2 the default in release 1.0 and then deprecate Py2 in 2.0. We'll just make sure to call it out in the release notes.
There was a problem hiding this comment.
@prschmid So does that mean you would like to move this PR to V2 as the default, and bump the version to 1.0? Or keep this PR as-is, with a follow-up 1.0 release with the switch to V2?
If the former, just let me know what else I need to do to get this thing merged in. 😄
There was a problem hiding this comment.
Sorry for the confusion. Let's keep the PR as is, so that once this is merged in to develop the V2 Zoom API will be the default. I will then create a new release of the client and label it as the V1 release of the zoomus library.
There was a problem hiding this comment.
Great! Then I'm ready whenever you are! 😄
| @@ -54,5 +50,31 @@ def test_requires_host_id(self): | |||
| context.exception.message, "'host_id' must be set") | |||
|
|
|||
|
|
|||
| class DeleteV2TestCase(unittest.TestCase): | |||
There was a problem hiding this comment.
To make sure this gets added to the suite properly, please add it to the suite() function at the top of the file. You'll want to do this with all of thew new V2 test cases you have made.
Also, to make it easier to read, can you change all of these test class names to follow the following pattern
[TestClassName]V1TestCase
[TestClassName]V2TestCase
I.e. this file should contain class DeleteV1TestCase and class class DeleteV2TestCase. The same should be applied to all test classes.
Once Zoom officially removes all support for their V1 API, we can simply go through and find all mentions of V1 and remove things.
There was a problem hiding this comment.
I was actually quite curious about this suite() convention. AFAICT, it is actually having no effect. In experimenting with it some, using tests.zoomus.test_utils as an example, I have confirmed that. It has the following code block:
def suite():
"""Define all the tests of the module."""
suite = unittest.TestSuite()
suite.addTest(unittest.makeSuite(ApiClientTestCase))
suite.addTest(unittest.makeSuite(RequireKeysTestCase))
suite.addTest(unittest.makeSuite(DateToStrTestCase))
suite.addTest(unittest.makeSuite(IsStrTypeTestCase))
return suiteIn running this test suite with that code block
- Fully in tact
- Fully commented out
- Partially commented out
I always get exactly the same results:
(zoomus) [jwilhelm@LMIT-JWILH zoomus]$ python -m unittest tests.zoomus.test_util
.....................................s.s......
----------------------------------------------------------------------
Ran 46 tests in 0.010s
OK (skipped=2)
(zoomus) [jwilhelm@LMIT-JWILH zoomus]$The same is also true when using tox and Python 2.7:
(zoomus) [jwilhelm@LMIT-JWILH zoomus]$ tox -e py27 -- -s tests.zoomus.test_util
GLOB sdist-make: /Users/jwilhelm/Documents/workspace/zoomus/setup.py
py27 inst-nodeps: /Users/jwilhelm/Documents/workspace/zoomus/.tox/.tmp/package/1/zoomus-0.1.8.zip
py27 installed: DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.,certifi==2019.3.9,chardet==3.0.4,coverage==4.5.3,funcsigs==1.0.2,idna==2.8,mock==2.0.0,nose==1.3.7,pbr==5.2.0,PyJWT==1.7.1,requests==2.21.0,six==1.12.0,urllib3==1.24.2,zoomus==0.1.8,zoomus2==0.1.8
py27 run-test-pre: PYTHONHASHSEED='979340893'
py27 run-test: commands[0] | nosetests -s tests.zoomus.test_util
..............................................
Name Stmts Miss Branch BrPart Cover
------------------------------------------------------------------
zoomus/__init__.py 5 0 0 0 100%
zoomus/client.py 42 0 2 0 100%
zoomus/components/__init__.py 2 0 0 0 100%
zoomus/components/base.py 12 0 4 0 100%
zoomus/components/meeting.py 48 0 10 0 100%
zoomus/components/recording.py 34 0 8 0 100%
zoomus/components/report.py 33 0 0 0 100%
zoomus/components/user.py 39 0 0 0 100%
zoomus/components/webinar.py 60 0 10 0 100%
zoomus/util.py 88 5 36 7 90%
------------------------------------------------------------------
TOTAL 363 5 70 7 97%
----------------------------------------------------------------------
Ran 46 tests in 0.015s
OK
__________________________________________________________________________________________________________________________________ summary ___________________________________________________________________________________________________________________________________
py27: commands succeeded
congratulations :)
(zoomus) [jwilhelm@LMIT-JWILH zoomus]$My best guess is, this pattern is coming from an example in the docs for both Python 2.7 and 3.x, which offers up the suggestion of a suite() method for customizing the discovery of unit tests. However, by default, the test discovery mechanism of Python's unittest module will actually discover all instances of unittest.TestCase, effectively performing exactly the behavior you are seeking here.
If you are truly trying to fully customize the test discovery and running behavior, it actually takes a good bit more effort. As an example, I have fully customized the test suite in the pyangbind library, starting with a line in setup.py: https://github.com/robshakir/pyangbind/blob/master/setup.py#L51, and then with a custom suite here: https://github.com/robshakir/pyangbind/blob/master/tests/__init__.py
But the customization actually stops there; the individual test modules have no extra customization, aside from the fact that they are named run.py instead of test_*.py.
So if anything, I would actually recommend dropping all of these suite() declarations, as they appear to be fully dead code.
There was a problem hiding this comment.
I'm fine either way. We were using the suite() convention in a larger project in which we were actually customizing the tests being run (e.g. only running subsets in particular situations, etc.). But since this project is much smaller and doesn't have a test suite that can take over an hour to complete, happy to have you remove them for the sake of simplicity.
|
|
||
| mock_post_request.assert_called_with( | ||
| "/meeting/get", | ||
| "/user/get", |
There was a problem hiding this comment.
Thanks for fixing all these copy and paste errors from the original test suite!
|
@prschmid I forgot to mention, I addressed a few concerns, and gave feedback on the others. 😄 |
This picks up where #10 left off.
So far I have:
actmdwheel.Still to do: