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 py3.5 compatible stubs to objector.py #1173

Merged
merged 2 commits into from
Dec 27, 2019
Merged

Add py3.5 compatible stubs to objector.py #1173

merged 2 commits into from
Dec 27, 2019

Conversation

PythonCoderAS
Copy link
Contributor

Parent: #1164

Type hints:

  • praw/objector.py

praw/objector.py Outdated
from .exceptions import APIException, ClientException
from .util import snake_case_keys
from typing import Dict, List, Any, Union, Optional, NoReturn, TypeVar
Copy link
Member

Choose a reason for hiding this comment

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

Can you move system-level imports above package-level ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

praw/objector.py Outdated Show resolved Hide resolved
@bboe bboe merged commit 6162c18 into praw-dev:master Dec 27, 2019
@bboe
Copy link
Member

bboe commented Dec 27, 2019

Thanks! 🎆

@PythonCoderAS PythonCoderAS deleted the type-objector branch December 27, 2019 14:55
Copy link
Contributor

@jarhill0 jarhill0 left a comment

Choose a reason for hiding this comment

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

I realize this has been merged already but I believe there's a mistake.

def check_error(cls, data):
def check_error(
cls, data: Union[List[Any], Dict[str, Dict[str, str]]]
) -> NoReturn:
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe usage of NoReturn here is incorrect. Per the documentation:

Special type indicating that a function never returns.

The check_error function sometimes never returns (when an exception is raised) but usually returns None (when no exception is raised). Type annotations should be updated to reflect this. I'm not familiar with type annotations, but perhaps a Union[NoneType, NoReturn] would be appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to just not use NoReturn so this should be phased out as well. I’m on a phone now so I’ll make a PR tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch @jarhill0. To be honest, I haven't really scrutinized these PRs at all. I figure it's all an improvement, even if it's not 100% perfect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants