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 more type annotations and improve MonkeyType tooling #600

Merged
merged 19 commits into from
Apr 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions mypy.ini
Original file line number Diff line number Diff line change
@@ -1,13 +1,32 @@
[mypy]
; TODO: check_untyped_defs = True
; TODO: Make this more granular; docs recommend it as a "last resort"
; https://mypy.readthedocs.io/en/latest/existing_code.html#start-small
; https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-type-hints-for-third-party-library
ignore_missing_imports = True
; NOTE: Docs recommend `normal` or `silent` for an existing codebase
; https://mypy.readthedocs.io/en/latest/running_mypy.html#following-imports
; follow_imports = skip

warn_redundant_casts = True
warn_unused_configs = True
warn_unused_ignores = True

; Reporting
show_traceback = True
html_report = mypy
linecount_report = mypy
lineprecision_report = mypy
txt_report = mypy

; TODO: Adopt --strict settings, iterating towards something like:
; https://github.com/pypa/packaging/blob/master/setup.cfg
; Starting with modules that have annotations applied via MonkeyType
[mypy-twine.auth,twine.cli,twine.package,twine.repository,twine.utils]
; Enabling this will fail on subclasses of untype imports, e.g. tqdm
; disallow_subclassing_any = True
disallow_any_generics = True
disallow_untyped_calls = True
disallow_untyped_defs = True
disallow_incomplete_defs = True
check_untyped_defs = True
disallow_untyped_decorators = True
no_implicit_optional = True
warn_return_any = True
no_implicit_reexport = True
strict_equality = True
8 changes: 8 additions & 0 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,14 @@ commands =
# TODO: Consider checking the tests after the source is fully typed
mypy twine

[testenv:monkeytype]
deps =
{[testenv]deps}
{[testenv:typing]deps}
monkeytype
commands =
monkeytype {posargs:run -m pytest}

[testenv:lint]
deps =
{[testenv:code-style]deps}
Expand Down
16 changes: 9 additions & 7 deletions twine/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import warnings
from typing import Callable
from typing import Optional
from typing import Type
from typing import cast

import keyring

Expand All @@ -11,18 +13,18 @@


class CredentialInput:
def __init__(self, username: str = None, password: str = None):
def __init__(self, username: str = None, password: str = None) -> None:
self.username = username
self.password = password


class Resolver:
def __init__(self, config: utils.RepositoryConfig, input: CredentialInput):
def __init__(self, config: utils.RepositoryConfig, input: CredentialInput) -> None:
self.config = config
self.input = input

@classmethod
def choose(cls, interactive):
def choose(cls, interactive: bool) -> Type["Resolver"]:
return cls if interactive else Private

@property # type: ignore # https://github.com/python/mypy/issues/1362
Expand Down Expand Up @@ -53,7 +55,7 @@ def get_username_from_keyring(self) -> Optional[str]:
try:
creds = keyring.get_credential(self.system, None)
if creds:
return creds.username
return cast(str, creds.username)
except AttributeError:
# To support keyring prior to 15.2
pass
Expand All @@ -63,7 +65,7 @@ def get_username_from_keyring(self) -> Optional[str]:

def get_password_from_keyring(self) -> Optional[str]:
try:
return keyring.get_password(self.system, self.username)
return cast(str, keyring.get_password(self.system, self.username))
except Exception as exc:
warnings.warn(str(exc))
return None
Expand All @@ -76,10 +78,10 @@ def password_from_keyring_or_prompt(self) -> str:
"password", getpass.getpass
)

def prompt(self, what: str, how: Callable) -> str:
def prompt(self, what: str, how: Callable[..., str]) -> str:
return how(f"Enter your {what}: ")


class Private(Resolver):
def prompt(self, what: str, how: Optional[Callable] = None) -> str:
def prompt(self, what: str, how: Optional[Callable[..., str]] = None) -> str:
raise exceptions.NonInteractive(f"Credential not found for {what}.")
14 changes: 10 additions & 4 deletions twine/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import argparse
from typing import Any
from typing import Dict
from typing import List
from typing import Tuple
Comment on lines +15 to +18
Copy link
Member

Choose a reason for hiding this comment

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

Definitely a nit, but is there a reason why we're importing like this and not:

Suggested change
from typing import Any
from typing import Dict
from typing import List
from typing import Tuple
from typing import Any, Dict, List, Tuple

Copy link
Member

Choose a reason for hiding this comment

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

Also, would be nice to avoid these imports at runtime, see what the packaging library is doing here: https://github.com/pypa/packaging/blob/61672bf9f507f38e84ce2786a1c42f55fa0a3153/packaging/_typing.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely a nit, but is there a reason why we're importing like this

That's a side-effect of:

twine/.isort.cfg

Lines 2 to 3 in 2c30b1e

# NOTE: Consider single_line_exclusions=typing in next version of isort
force_single_line=True

which was added in #576. Still waiting on an isort release that includes PyCQA/isort#1085.

would be nice to avoid these imports at runtime

I've seen the if TYPE_CHECKING pattern before, but never investigated why it was useful. From the packaging link (emphasis mine):

Generally, typing would be imported at runtime and used in that fashion -
it acts as a no-op at runtime and does not have any run-time overhead by
design. As it turns out, typing uses separate sources for Python 2/Python 3. To work around this, mypy allows the typing import to be behind a False-y optional.

My read on this is that if TYPE_CHECKING is useful for Py2/3 compatibility, which Twine doesn't need. The mypy docs also include examples of using it for troubleshooting, e.g. import cycles.

So, I'm happy to add it if it's valuable, but it's not clear to me how it would be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so if I understand this correctly, as per the google style guide , we are allowed to import multiple specific classes on one line from the typing module.

But there is currently no support from isort in the version we are using to globally force single line imports, but exclude certain modules from that rule, which is what single_line_exclusions=typing would do in a newer version of isort once PyCQA/isort#1085 is merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deveshks That's correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@di I'm going to merge this as-is, but I'd love to hear your thoughts about TYPE_CHECKING.

Also: I really enjoyed your Static Typing talk.


import pkg_resources
import pkginfo
Expand All @@ -24,12 +28,14 @@
from twine import _installed


def _registered_commands(group="twine.registered_commands"):
def _registered_commands(
group: str = "twine.registered_commands",
) -> Dict[str, pkg_resources.EntryPoint]:
registered_commands = pkg_resources.iter_entry_points(group=group)
return {c.name: c for c in registered_commands}


def list_dependencies_and_versions():
def list_dependencies_and_versions() -> List[Tuple[str, str]]:
return [
("pkginfo", _installed.Installed(pkginfo).version),
("requests", requests.__version__),
Expand All @@ -39,13 +45,13 @@ def list_dependencies_and_versions():
]


def dep_versions():
def dep_versions() -> str:
return ", ".join(
"{}: {}".format(*dependency) for dependency in list_dependencies_and_versions()
)


def dispatch(argv):
def dispatch(argv: List[str]) -> Any:
registered_commands = _registered_commands()
parser = argparse.ArgumentParser(prog="twine")
parser.add_argument(
Expand Down
8 changes: 5 additions & 3 deletions twine/package.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,14 +159,16 @@ def metadata_dictionary(self) -> Dict[str, MetadataValue]:

return data

def add_gpg_signature(self, signature_filepath: str, signature_filename: str):
def add_gpg_signature(
self, signature_filepath: str, signature_filename: str
) -> None:
if self.gpg_signature is not None:
raise exceptions.InvalidDistribution("GPG Signature can only be added once")

with open(signature_filepath, "rb") as gpg:
self.gpg_signature = (signature_filename, gpg.read())

def sign(self, sign_with: str, identity: Optional[str]):
def sign(self, sign_with: str, identity: Optional[str]) -> None:
print(f"Signing {self.basefilename}")
gpg_args: Tuple[str, ...] = (sign_with, "--detach-sign")
if identity:
Expand All @@ -177,7 +179,7 @@ def sign(self, sign_with: str, identity: Optional[str]):
self.add_gpg_signature(self.signed_filename, self.signed_basefilename)

@classmethod
def run_gpg(cls, gpg_args):
def run_gpg(cls, gpg_args: Tuple[str, ...]) -> None:
try:
subprocess.check_call(gpg_args)
return
Expand Down
22 changes: 14 additions & 8 deletions twine/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,13 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import sys
from typing import Any
from typing import Dict
from typing import List
from typing import Optional
from typing import Set
from typing import Tuple
from typing import cast

import requests
import requests_toolbelt
Expand All @@ -38,7 +41,7 @@


class ProgressBar(tqdm.tqdm):
def update_to(self, n):
def update_to(self, n: int) -> None:
"""Update the bar in the way compatible with requests-toolbelt.

This is identical to tqdm.update, except ``n`` will be the current
Expand Down Expand Up @@ -68,7 +71,8 @@ def __init__(
for scheme in ("http://", "https://"):
self.session.mount(scheme, self._make_adapter_with_retries())

self._releases_json_data: Dict[str, Dict] = {}
# Working around https://github.com/python/typing/issues/182
self._releases_json_data: Dict[str, Dict[str, Any]] = {}
self.disable_progress_bar = disable_progress_bar

@staticmethod
Expand All @@ -86,14 +90,16 @@ def _make_user_agent_string() -> str:
from twine import cli

dependencies = cli.list_dependencies_and_versions()
return (
user_agent.UserAgentBuilder("twine", twine.__version__,)
user_agent_string = (
user_agent.UserAgentBuilder("twine", twine.__version__)
.include_extras(dependencies)
.include_implementation()
.build()
)

def close(self):
return cast(str, user_agent_string)

def close(self) -> None:
self.session.close()

@staticmethod
Expand All @@ -117,7 +123,7 @@ def set_client_certificate(self, clientcert: Optional[str]) -> None:
if clientcert:
self.session.cert = clientcert

def register(self, package):
def register(self, package: package_file.PackageFile) -> requests.Response:
data = package.metadata_dictionary()
data.update({":action": "submit", "protocol_version": "1"})

Expand Down Expand Up @@ -232,7 +238,7 @@ def package_is_uploaded(

return False

def release_urls(self, packages):
def release_urls(self, packages: List[package_file.PackageFile]) -> Set[str]:
if self.url.startswith(WAREHOUSE):
url = WAREHOUSE_WEB
elif self.url.startswith(TEST_WAREHOUSE):
Expand All @@ -245,7 +251,7 @@ def release_urls(self, packages):
for package in packages
}

def verify_package_integrity(self, package: package_file.PackageFile):
def verify_package_integrity(self, package: package_file.PackageFile) -> None:
# TODO(sigmavirus24): Add a way for users to download the package and
# check it's hash against what it has locally.
pass
13 changes: 9 additions & 4 deletions twine/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,17 @@ def __init__(
)

@property
def username(self):
return self.auth.username
def username(self) -> Optional[str]:
# Workaround for https://github.com/python/mypy/issues/5858
return cast(Optional[str], self.auth.username)

@property
def password(self):
return None if self.client_cert else self.auth.password
def password(self) -> Optional[str]:
if self.client_cert:
return None

# Workaround for https://github.com/python/mypy/issues/5858
return cast(Optional[str], self.auth.password)

@staticmethod
def register_argparse_arguments(parser: argparse.ArgumentParser) -> None:
Expand Down
33 changes: 26 additions & 7 deletions twine/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@
import functools
import os
import os.path
from typing import Any
from typing import Callable
from typing import DefaultDict
from typing import Dict
from typing import Optional
from typing import Sequence
from typing import Union
from urllib.parse import urlparse
from urllib.parse import urlunparse

Expand Down Expand Up @@ -181,7 +184,7 @@ def get_userpass_value(
cli_value: Optional[str],
config: RepositoryConfig,
key: str,
prompt_strategy: Optional[Callable] = None,
prompt_strategy: Optional[Callable[[], str]] = None,
) -> Optional[str]:
"""Gets the username / password from config.

Expand Down Expand Up @@ -221,33 +224,49 @@ class EnvironmentDefault(argparse.Action):
"""Get values from environment variable."""

def __init__(
self, env: str, required: bool = True, default: Optional[str] = None, **kwargs
self,
env: str,
required: bool = True,
default: Optional[str] = None,
**kwargs: Any,
) -> None:
default = os.environ.get(env, default)
self.env = env
if default:
required = False
super().__init__(default=default, required=required, **kwargs)

def __call__(self, parser, namespace, values, option_string=None):
def __call__(
self,
parser: argparse.ArgumentParser,
namespace: argparse.Namespace,
values: Union[str, Sequence[Any], None],
option_string: Optional[str] = None,
) -> None:
setattr(namespace, self.dest, values)


class EnvironmentFlag(argparse.Action):
"""Set boolean flag from environment variable."""

def __init__(self, env: str, **kwargs) -> None:
def __init__(self, env: str, **kwargs: Any) -> None:
default = self.bool_from_env(os.environ.get(env))
self.env = env
super().__init__(default=default, nargs=0, **kwargs)

def __call__(self, parser, namespace, values, option_string=None):
def __call__(
self,
parser: argparse.ArgumentParser,
namespace: argparse.Namespace,
values: Union[str, Sequence[Any], None],
option_string: Optional[str] = None,
) -> None:
setattr(namespace, self.dest, True)

@staticmethod
def bool_from_env(val):
def bool_from_env(val: Optional[str]) -> bool:
"""
Allow '0' and 'false' and 'no' to be False
"""
falsey = {"0", "false", "no"}
return val and val.lower() not in falsey
return bool(val and val.lower() not in falsey)