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

Add type annotations to storage API #1410

Merged
merged 6 commits into from Jan 20, 2020
Merged

Add type annotations to storage API #1410

merged 6 commits into from Jan 20, 2020

Conversation

c-w
Copy link
Member

@c-w c-w commented Jan 14, 2020

Add type annotations to storage API

Description

As discussed in #1408 (comment), this PR adds type annotations to the StorageDriver API.

For now, mypy errors are ignored for the s3, google_storage and cloudfiles drivers since the use of inheritance on the connection classes is causing issues which will require a more thorough strategy to address.

Status

  • done, ready for review

Checklist

@codecov-io
Copy link

@codecov-io codecov-io commented Jan 14, 2020

Codecov Report

Merging #1410 into trunk will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            trunk    #1410      +/-   ##
==========================================
+ Coverage   86.46%   86.46%   +<.01%     
==========================================
  Files         366      366              
  Lines       76791    76794       +3     
  Branches     7529     7529              
==========================================
+ Hits        66396    66399       +3     
  Misses       7527     7527              
  Partials     2868     2868
Impacted Files Coverage Δ
libcloud/storage/drivers/nimbus.py 0% <ø> (ø) ⬆️
libcloud/storage/drivers/local.py 73.26% <ø> (ø) ⬆️
libcloud/storage/drivers/dummy.py 50.83% <ø> (ø) ⬆️
libcloud/storage/providers.py 57.14% <100%> (+7.14%) ⬆️
libcloud/storage/drivers/s3.py 89.34% <100%> (ø) ⬆️
libcloud/storage/base.py 82.03% <100%> (ø) ⬆️
libcloud/storage/drivers/oss.py 70.55% <100%> (ø) ⬆️
libcloud/storage/drivers/azure_blobs.py 85.62% <100%> (ø) ⬆️
libcloud/common/aws.py 84.48% <100%> (ø) ⬆️
libcloud/storage/drivers/backblaze_b2.py 87.87% <100%> (ø) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb865b1...5e3ef03. Read the comment docs.

"""
raise NotImplementedError(
'download_object_as_stream not implemented for this driver')

def upload_object(self, file_path, container, object_name, extra=None,
verify_hash=True, headers=None):
# type: (str, Container, str, Optional[dict], Optional[bool], Optional[dict]) -> Object # noqa: E501
Copy link
Member

@Kami Kami Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since verify_hash has a default value, I don't think it needs Optional annotation.

Copy link
Member

@Kami Kami Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for headers we could probably use Dict[str, str].

Copy link
Member Author

@c-w c-w Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d80b17c.

@Kami
Copy link
Member

@Kami Kami commented Jan 15, 2020

Nice work 👍


What's your take on utilizing Python 3 native syntax for type annotations vs using comments?

In theory, using comments, allows the trunk to still work with Python 2.7, but since we don't officially support it any more, I'm not sure if that matters or not since sooner or later we will likely start utilizing other Python 3 only features as well (and we have v2.8.x branch we can base v2.8.x bug fix release on, if needed).

@c-w
Copy link
Member Author

@c-w c-w commented Jan 15, 2020

What's your take on utilizing Python 3 native syntax for type annotations vs using comments?

@Kami Personally I'm not a huge fan of the type comments for function signatures and would be in favor of eventually switching to the native type annotations to increase readability.

E.g. compare the following two snippets:

def upload_object_via_stream(self, iterator, object_name, extra=None,
                             headers=None):
    # type: (Iterator[bytes], str, Optional[dict], Optional[Dict[str, str]]) -> Object  # noqa: E501
def upload_object_via_stream(self,
                             iterator: Iterator[bytes],
                             object_name: str,
                             extra: Optional[dict] = None,
                             headers: Optional[Dict[str, str]] = None,
                             ) -> Object:

From my point of view, the former is harder to read since the function arguments must be matched with the type declaration based on index/position as opposed to being attached to each argument explicitly.

@c-w
Copy link
Member Author

@c-w c-w commented Jan 15, 2020

The type annotations uncovered a bug: most drivers were discarding the base API's headers argument in upload_object and upload_object_via_stream. I fixed this in 3b6cdc9.

Kami
Kami approved these changes Jan 20, 2020
@Kami
Copy link
Member

@Kami Kami commented Jan 20, 2020

@c-w Good catch on the headers issue 👍

As far as comments notation goes - in-line annotations can also be used when using comments, but I also agree that native notation is nicer.

So perhaps now that we don't need to support Python 2.7 anymore, we can just switch to that notation.

@c-w c-w merged commit acb9a95 into apache:trunk Jan 20, 2020
1 check passed
@c-w c-w deleted the storage-type-annotations branch Jan 20, 2020
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.

None yet

3 participants