Skip to content

Added typing annotations to io/__init__ #4224

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

Merged
merged 6 commits into from
Aug 31, 2021
Merged

Conversation

frgfm
Copy link
Contributor

@frgfm frgfm commented Jul 29, 2021

Following up on #2025, this PR adds missing typing annotations in io/__init__.py.

Note to reviewers: I'm not sure about the return types when a method returns self 🤷‍♂️

Any feedback is welcome!

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

@frgfm I changed the return type for self. See my commit.

return self

def seek(self, time_s: float):
def seek(self, time_s: float) -> 'VideoReader':
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be replaced with VideoReader once we drop Python 3.6 support. See here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.
I learn something new everyday because of review from torchvision maintainers 😇

There are couple of more places where I have used this workaround in typing in torchvision. I will keep a note of this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@datumbox yup fully agree! But for now (this PR), I guess we keep the string version?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that's right, nothing we can do for now.

@@ -126,10 +127,10 @@ def __next__(self):
raise StopIteration
return {"data": frame, "pts": pts}

def __iter__(self):
def __iter__(self) -> Iterator['VideoReader']:
Copy link
Contributor

Choose a reason for hiding this comment

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

See mypy doc.

@datumbox datumbox requested review from NicolasHug and fmassa August 16, 2021 09:39
@datumbox
Copy link
Contributor

@frgfm could you please merge master to clear the CI errors?

@frgfm
Copy link
Contributor Author

frgfm commented Aug 28, 2021

@datumbox all errors were cleared after the previous merge 👌
I just merged main so have everything ready for review & I guess merge 👍

@frgfm frgfm requested a review from datumbox August 28, 2021 10:52
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@datumbox datumbox merged commit a8d2227 into pytorch:main Aug 31, 2021
@frgfm frgfm deleted the io-init-typing branch August 31, 2021 13:39
facebook-github-bot pushed a commit that referenced this pull request Sep 9, 2021
Summary:
* style: Added typing annotations

* Specified types for iter and seek.

Reviewed By: fmassa

Differential Revision: D30793319

fbshipit-source-id: b5d3a220639d239f64cee6712aa07e19fdaaf875

Co-authored-by: Vasilis Vryniotis <[email protected]>
Co-authored-by: Vasilis Vryniotis <[email protected]>
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.

5 participants