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.

Sign up for GitHub

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

Jump to bottom

Add type annotations to twine.commands.check #608

Merged
merged 3 commits into from
Apr 27, 2020
Merged

Add type annotations to twine.commands.check #608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
703414e
Apply monkeytype to twine.commands.check
bhrutledge Apr 25, 2020
4879261
Clean up after monkeytype
bhrutledge Apr 25, 2020
ebd2ada
Simplify metadata typing
bhrutledge Apr 26, 2020
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
2 changes: 1 addition & 1 deletion mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ 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.exceptions,twine.package,twine.repository,twine.utils,twine.commands,twine.wheel,twine.wininst,twine.commands.register]
[mypy-twine.auth,twine.cli,twine.exceptions,twine.package,twine.repository,twine.utils,twine.wheel,twine.wininst,twine.commands]
; Enabling this will fail on subclasses of untype imports, e.g. tqdm
; disallow_subclassing_any = True
disallow_any_generics = True
Expand Down
8 changes: 4 additions & 4 deletions tests/test_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def test_check_no_distributions(monkeypatch):

monkeypatch.setattr(commands, "_find_dists", lambda a: [])

assert not check.check("dist/*", output_stream=stream)
assert not check.check(["dist/*"], output_stream=stream)
assert stream.getvalue() == "No files to check.\n"


Expand All @@ -72,7 +72,7 @@ def test_check_passing_distribution(monkeypatch):
)
monkeypatch.setattr(check, "_WarningStream", lambda: warning_stream)

assert not check.check("dist/*", output_stream=output_stream)
assert not check.check(["dist/*"], output_stream=output_stream)
bhrutledge marked this conversation as resolved.
Show resolved Hide resolved
assert output_stream.getvalue() == "Checking dist/dist.tar.gz: PASSED\n"
assert renderer.render.calls == [pretend.call("blah", stream=warning_stream)]

Expand All @@ -94,7 +94,7 @@ def test_check_no_description(monkeypatch, capsys):

# used to crash with `AttributeError`
output_stream = io.StringIO()
check.check("dist/*", output_stream=output_stream)
check.check(["dist/*"], output_stream=output_stream)
assert output_stream.getvalue() == (
"Checking dist/dist.tar.gz: PASSED, with warnings\n"
" warning: `long_description_content_type` missing. "
Expand Down Expand Up @@ -123,7 +123,7 @@ def test_check_failing_distribution(monkeypatch):
)
monkeypatch.setattr(check, "_WarningStream", lambda: warning_stream)

assert check.check("dist/*", output_stream=output_stream)
assert check.check(["dist/*"], output_stream=output_stream)
assert output_stream.getvalue() == (
"Checking dist/dist.tar.gz: FAILED\n"
" `long_description` has syntax errors in markup and would not be "
Expand Down
42 changes: 19 additions & 23 deletions twine/commands/check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@
import io
import re
import sys
import textwrap
from typing import IO
from typing import List
from typing import Tuple
from typing import cast

import readme_renderer.rst

Expand Down Expand Up @@ -43,10 +48,10 @@


class _WarningStream:
def __init__(self):
def __init__(self) -> None:
self.output = io.StringIO()

def write(self, text):
def write(self, text: str) -> None:
matched = _REPORT_RE.search(text)

if not matched:
Expand All @@ -61,20 +66,22 @@ def write(self, text):
)
)

def __str__(self):
def __str__(self) -> str:
return self.output.getvalue()


def _check_file(filename, render_warning_stream):
def _check_file(
filename: str, render_warning_stream: _WarningStream
) -> Tuple[List[str], bool]:
"""Check given distribution."""
warnings = []
is_ok = True

package = package_file.PackageFile.from_filename(filename, comment=None)

metadata = package.metadata_dictionary()
description = metadata["description"]
description_content_type = metadata["description_content_type"]
description = cast(str, metadata["description"])
description_content_type = cast(str, metadata["description_content_type"])

if description_content_type is None:
warnings.append(
Expand All @@ -97,18 +104,7 @@ def _check_file(filename, render_warning_stream):
return warnings, is_ok


# TODO: Replace with textwrap.indent when Python 2 support is dropped
def _indented(text, prefix):
bhrutledge marked this conversation as resolved.
Show resolved Hide resolved
"""Adds 'prefix' to all non-empty lines on 'text'."""

def prefixed_lines():
for line in text.splitlines(True):
yield (prefix + line if line.strip() else line)

return "".join(prefixed_lines())


def check(dists, output_stream=sys.stdout):
def check(dists: List[str], output_stream: IO[str] = sys.stdout) -> bool:
uploads = [i for i in commands._find_dists(dists) if not i.endswith(".asc")]
if not uploads: # Return early, if there are no files to check.
output_stream.write("No files to check.\n")
Expand All @@ -130,8 +126,8 @@ def check(dists, output_stream=sys.stdout):
"`long_description` has syntax errors in markup and "
"would not be rendered on PyPI.\n"
)
output_stream.write(_indented(error_text, " "))
output_stream.write(_indented(str(render_warning_stream), " "))
output_stream.write(textwrap.indent(error_text, " "))
output_stream.write(textwrap.indent(str(render_warning_stream), " "))
elif warnings:
output_stream.write("PASSED, with warnings\n")
else:
Expand All @@ -144,7 +140,7 @@ def check(dists, output_stream=sys.stdout):
return failure


def main(args):
def main(args: List[str]) -> bool:
parser = argparse.ArgumentParser(prog="twine check")
parser.add_argument(
"dists",
Expand All @@ -153,7 +149,7 @@ def main(args):
help="The distribution files to check, usually dist/*",
)

args = parser.parse_args(args)
parsed_args = parser.parse_args(args)

# Call the check function with the arguments from the command line
return check(args.dists)
return check(parsed_args.dists)
3 changes: 1 addition & 2 deletions twine/package.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import io
import os
import subprocess
from typing import IO
from typing import Dict
from typing import Optional
from typing import Sequence
Expand Down Expand Up @@ -46,7 +45,7 @@
".zip": "sdist",
}

MetadataValue = Union[str, Sequence[str], Tuple[str, IO, str]]
Copy link
Member

Choose a reason for hiding this comment

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

Why are we getting less specific here?

Copy link
Contributor Author

@bhrutledge bhrutledge Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In fa5ba3b, I used a cast on one of the dictionary values:

    metadata = package.metadata_dictionary()
    description = metadata["description"]
    description_content_type = metadata["description_content_type"]

    # ...

    content_type, params = cgi.parse_header(cast(str, description_content_type))

Because cgi.parse_header expects a str, not the Union[...]. This led me to investigate how the MetadataValue alias is used. One place is Repository._convert_data_to_list_of_tuples:

twine/twine/repository.py

Lines 105 to 108 in 7111240

@staticmethod
def _convert_data_to_list_of_tuples(
data: Dict[str, package_file.MetadataValue]
) -> List[Tuple[str, package_file.MetadataValue]]:

However, to my eye, nothing about that code requires the strictness of MetadataValue.

Also, I've seen the Dict[str, Any] idiom recommended by Guido for dictionaries with mixed types(e.g. JSON) and I've used it in at least one other place:

twine/twine/repository.py

Lines 74 to 75 in 8765d14

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

Ultimately, this change feels more readable/comprehensible to me. I think the ideal solution might be TypedDict in Py 3.8 and/or type hints for pkginfo.

Copy link
Member

Choose a reason for hiding this comment

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

That issue seems to be more tied to JSON than anything (by my skimming). I'd argue that Any is a barely valuable annotation and its main value is in "I haven't figured out what this should be right now". I don't understand how casting description_content_type to a string led us down this road, because that's exactly one of the values.

I would also think that pkginfo is popular enough that there should already be some stubs for it, if not, it still seems like a valuable thing to have

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 don't understand how casting description_content_type to a string led us down this road, because that's exactly one of the values.

I've been trying to avoid cast as a smell, though I have used it in a few places where mypy required it, e.g. when we know an Optional[str] is actually a str:

twine/twine/commands/upload.py

Line 60 in 2cec216

repository_url = cast(str, upload_settings.repository_config["repository"])

I'd argue that Any is a barely valuable annotation and its main value is in "I haven't figured out what this should be right now".

I think it provides value to the reader indicating that "this value could be anything, so be careful". I also think how widely it's used is a matter of style, and it's more common when applying annotations to existing codebases.

https://mypy.readthedocs.io/en/stable/kinds_of_types.html#the-any-type
https://mypy.readthedocs.io/en/stable/casts.html

Do you feel strongly about this? I don't see much value in being so specific here, because it doesn't tell the reader or mypy enough to know what the runtime value of these dictionary keys will actually be. To me, that makes it just another thing that the reader has to look up.

I would also think that pkginfo is popular enough that there should already be some stubs for it, if not, it still seems like a valuable thing to have

Looking into this (and other missing imports) is on the roadmap for subsequent PR's: #231 (comment)

Copy link
Contributor

@deveshks deveshks Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I also have used cast in a similar way in https://github.com/pypa/twine/blob/master/twine/commands/register.py#L25 while adding type annotations.

I was wondering if assigning a value to repository_url if it is None might be a good idea rather than casting it. (I have assigned a value 'any', but I think any other standard value might be applicable here.

 repository_url = register_settings.repository_config["repository"]
    if repository_url is None:
        repository_url = 'any'

I also saw a similar approach taking place in twine/wininst.py

twine/twine/wininst.py

Lines 19 to 25 in d0ccb0c

@property
def py_version(self) -> str:
m = wininst_file_re.match(self.filename)
if m is None:
return "any"
else:
return m.group("pyver")

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel strongly enough to block the PR, but I would like us to converge, eventually, on types that are of use to us and anyone else even if they seem to not convey they actual value of a dictionary. In general, I'd say something is better than nothing.

I'm also worried that we've released MetadataValue and people are using twine.package (which I think is a documented part of the API) and thus we're causing problems for others usin gmypy and our bare-bones annotations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(One of these days I'll anticipate the API argument)

Just pushed ebd2ada, which feels like a good compromise to me. I kept the relaxed signature of _convert_data_to_list_of_tuples:

twine/twine/repository.py

Line 106 in ebd2ada

def _convert_data_to_list_of_tuples(data: Dict[str, Any]) -> List[Tuple[str, Any]]:

That removed the need for the awkward Tuple[str, IO, str], which is never actually stored in metadata_dictionary, but rather appended to the list of tuples:

twine/twine/repository.py

Lines 152 to 159 in ebd2ada

data_to_send = self._convert_data_to_list_of_tuples(data)
print(f"Uploading {package.basefilename}")
with open(package.filename, "rb") as fp:
data_to_send.append(
("content", (package.basefilename, fp, "application/octet-stream"),)
)

Leaving us with:

twine/twine/package.py

Line 48 in ebd2ada

MetadataValue = Union[str, Sequence[str]]

So, yeah, glad I took a closer look at this. Thanks. 😉

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if assigning a value to repository_url if it is None might be a good idea rather than casting it. (I have assigned a value 'any', but I think any other standard value might be applicable here.

So we've had complaints from people that they've uploaded things they didn't intend to as well as to places they didn't intend to. Placing a standard value in repository_url to satisfy mypy doesn't seem safe to me. Sorry I didn't see your comment earlier, @deveshks

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 I think your use of cast is appropriate, and I've used it that way as well.

Basically, given my_dict: Dict[str, Optional[str]], with cast(str, my_dict["key"]), we're saying: "at this point in the code, I'm confident that a) "key" is in my_dict and b) it's value is a str and not None". Having that confidence requires us to reason about the logic up to that point, but in the cases I've seen so far, it's seemed safe (and sometimes obvious from the context).

MetadataValue = Union[str, Sequence[str]]


class PackageFile:
Expand Down
4 changes: 1 addition & 3 deletions twine/repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,7 @@ def close(self) -> None:
self.session.close()

@staticmethod
def _convert_data_to_list_of_tuples(
data: Dict[str, package_file.MetadataValue]
) -> List[Tuple[str, package_file.MetadataValue]]:
def _convert_data_to_list_of_tuples(data: Dict[str, Any]) -> List[Tuple[str, Any]]:
data_to_send = []
for key, value in data.items():
if key in KEYWORDS_TO_NOT_FLATTEN or not isinstance(value, (list, tuple)):
Expand Down