Code review

[kracekumar]

@jbedorf and @MrFlynn. The overall code looks good. There are few minor suggested changes by me. @jvanheugten please add your reviews too. Please feel to modify or ignore. Though, I haven't tested the workflow by setting up the repo.

Typing

• https://github.com/minds-ai/zoom-drive-connector/blob/master/main.py#L32. The function returns a list, should be annotated as List[Dict[str, str]]. If you find difficult to annotate a dict, consider using namedtuple.
• Union[int, None] can also be written as Optional[int]. https://github.com/minds-ai/zoom-drive-connector/blob/master/zoom/zoom_api_exception.py#L24
• In https://github.com/minds-ai/zoom-drive-connector/blob/master/zoom/zoom_api.py#L131, return type should be Dict[] and not dict. date and filename in the dictionary can be None on failure, and helps to maintain the same structure.

Requests

• In https://github.com/minds-ai/zoom-drive-connector/blob/master/zoom/zoom_api.py#L68,

   if res.status_code == 401:
      # Handle unauthenticated requests.
      raise ZoomAPIException(401, 'Unauthorized', res.request, 'Not authenticated.')
    elif res.status_code == 409:
      # Handle error where content may have been removed already.
      raise ZoomAPIException(409, 'Resource Conflict', res.request, 'File deleted already.')

the above code can be written as

status_code = res.status_code 
if 400 <= status_code <= 499:
   message = {401: 'Not authenticated.', 409: 'File deleted already.')
   raise ZoomAPIException(status_code, res.reason, res.request, message.get(status_code, '')

Response API: http://docs.python-requests.org/en/master/api/#requests.Response

• https://github.com/minds-ai/zoom-drive-connector/blob/master/zoom/zoom_api.py#L98, the else block will run even if the status code is 500 which means their server failed. This block should be executed if the status_code is 2XX, you can check if res.ok or status code, so ideal flow could be something like

if 200 <= status_code <= 299:
# All well
elif 300 <= status_code <= 399:
# Server is redirecting, we can't do much
elif 400 <= status_code <= 499:
# Something we did wrong,
elif 500 <= status_code <= 599:
# Zoom Server Error
else:
# Never seen this but possible

General

• 'HTTP_STATUS: {c}-{n}, {m}'.format(c=self.status_code, n=self.name, m=self.message) rather than format string you can consider using f-string like f'HTTP_STATUS: {self.status_code}-{self.name}, {self.message}'.
• As a general practice __repr__ prints object creation method like Pipeline(steps=[(a, A())]). https://docs.python.org/3/library/functions.html#repr. https://github.com/minds-ai/zoom-drive-connector/blob/master/zoom/zoom_api_exception.py#L47
• Doc string is slightly different from our code base style. Our code base follows

def download_recording(self, url: str) -> str:
    """Downloads video file from Zoom to local folder.
    Args:
      url: Download URL for meeting recording.

    Returns: Path to the recording
    """

this repo follows

def download_recording(self, url: str) -> str:
    """Downloads video file from Zoom to local folder.
    :param url: Download URL for meeting recording.
    :return: Path to the recording
    """

• The URLS for various services are declared at multiple places. It can be put under one file with multiple classes like

class ZoomURLS(Enum): recordings: 'https://api.zoom.us/v2/meetings/{id}/recordings' delete_recordings: 'https://api.zoom.us/v2/meetings/{id}/recordings/{rid}' ...

The advantage of the method is, when zoom changes it's API, we can find all the URLS in one place and modify.

### Typos

- `entries` is misspelled as `entrees` in README.

### Testing

We can write unit tests for the code, we have written like ConfigParsers and other services specific code. One can mock the request, and response using [Python Mock library](https://docs.python.org/3/library/unittest.mock.html), [Responses](https://github.com/getsentry/responses). Once you have a list of scenarios like `Zoom recording file is missing`, `record files are available` then write tests for specific code parts.

[jvanheugten]

Not much to add myself. I agree with Krace that moving URLs to single location is useful, see also: https://github.com/minds-ai/zoom-drive-connector/blob/00e4c10e78b5346a4ec1153765023dcb5baa81ae/drive/drive_api.py#L42 . We might want to use the same bin/git and style_check.sh/.pylintrc/.pycodestyle as in the other repositories. Lots of typing with list, dict, etc., but normally you would use typing's List and Dict etc.

Maybe run:

pip install pipreqs pip-tools
pipreqs --force ./; mv requirements.txt requirements.in; pip-compile

to get a more reproducible set of dependencies (with versions).

From a security perspective, I don't know if it is great to have a plain readable config.yaml that gives access to drive, zoom, and slack.

[MrFlynn]

Thanks for the comments, everyone. Most of this stuff should be fairly easy to fix, though I have a couple of comments.

https://github.com/minds-ai/zoom-drive-connector/blob/master/main.py#L32. The function returns a list, should be annotated as List[Dict[str, str]]. If you find difficult to annotate a dict, consider using namedtuple.

@kracekumar I generally like to avoid namedtuples due to performance considerations (slow startup times). This is a fairly small application so it wouldn't matter here, but it's a practice I like to follow.

In terms of docstrings, I've been writing them in reStructured Text format (as recommended by PEP 287). The reason for this is that this repository was started before we had a stated company-wide docstring format. As a consequence, most of the Python code written by IT uses this format now. I think this is part of a bigger discussion on whether to migrate IT's docstring formatting to something else.

@jvanheugten In terms of having access keys and credentials in a file, there's a tradeoff to be made here: either keeping everything on disk or using environment variables (which get logged in your shell history anyway). The configuration file will likely be in someone's home folder which should only be accessible by one person.

[MrFlynn]

So here's a preliminary todo list. Feel free to comment on this so I can add stuff.

• [x] Update type annotations to use proper/more explicit format.
• [x] Update requests error handling code blocks (w.r.t. what Krace wrote in the original comment in this issue).
• [x] Move API URLs to enums to reduce duplicate strings in code.
• [x] Fix typo in README.
• [x] Implement testing using mock unittests or responses library (personally I prefer the former because it's built-in but I'm open to recommendations).

[MrFlynn]

All changes applied in #16 and #17.