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

Adding missing docstrings to twine/commands #799

Merged
merged 17 commits into from Sep 20, 2021
Merged
1 change: 0 additions & 1 deletion .flake8
Expand Up @@ -13,5 +13,4 @@ per-file-ignores =
# D103 Missing docstring in public function
# D104 Missing docstring in public package
twine/*: D100,D101,D102,D103,D104
twine/commands/*: D100,D101,D102,D103,D104
tests/*: D100,D101,D102,D103,D104
1 change: 1 addition & 0 deletions twine/commands/__init__.py
@@ -1,3 +1,4 @@
"""Initialize the twine.commands package."""
bhrutledge marked this conversation as resolved.
Show resolved Hide resolved
# Copyright 2013 Donald Stufft
#
# Licensed under the Apache License, Version 2.0 (the "License");
Expand Down
40 changes: 39 additions & 1 deletion twine/commands/check.py
@@ -1,3 +1,8 @@
"""Module containing the check function in twine.

This module checks file(s) that users are going to
upload, and raises warnings when errors occurred.
"""
# Copyright 2018 Dustin Ingram
#
# Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -70,7 +75,6 @@ def __str__(self) -> str:
def _check_file(
filename: str, render_warning_stream: _WarningStream
) -> Tuple[List[str], bool]:
"""Check given distribution."""
warnings = []
is_ok = True

Expand Down Expand Up @@ -106,6 +110,32 @@ def check(
output_stream: IO[str] = sys.stdout,
strict: bool = False,
) -> bool:
"""Check the file(s) given by user.

If there are no files to check, ``False`` is returned with a output.

If the check is failed, error text is given and ``failure = True`` is returned.

If strict is set to ``True`` by user, the check will fail when there are warnings.
Otherwise, the check will pass with warnings.

If the check is passed, message for passing the check is given
and ``failure = False`` is returned.

:param List[str] dists:
The distribution files we are going to check.
:param IO[str] output_stream:
Output stream of the check.

This defaults to ``sys.stdout``.
:param: bool strict:
dukecat0 marked this conversation as resolved.
Show resolved Hide resolved
Strict mode for the check.

This defaults to ``False``.

:return bool:
Determine whether the check has passed.
"""
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 Down Expand Up @@ -146,6 +176,14 @@ def check(


def main(args: List[str]) -> bool:
"""Entry-point of check command.

:param List[str] args:
Arguments for the check command.

:return bool:
The result of the check function.
"""
parser = argparse.ArgumentParser(prog="twine check")
parser.add_argument(
"dists",
Expand Down
18 changes: 18 additions & 0 deletions twine/commands/register.py
@@ -1,3 +1,7 @@
"""Module containing the register function in twine.

This module registers the package to the repository.
"""
# Copyright 2015 Ian Cordasco
#
# Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -21,6 +25,15 @@


def register(register_settings: settings.Settings, package: str) -> None:
"""Register package to repository.

Raises:
dukecat0 marked this conversation as resolved.
Show resolved Hide resolved
:class:`exceptions.PackageNotFound`: If the package is not existing.
:class:`exceptions.RedirectDetected.from_args`: If we get a redirect.

Finally, it raises for status. If error is occurred, it will print
a error message in verbose output.
"""
repository_url = cast(str, register_settings.repository_config["repository"])
print(f"Registering package to {repository_url}")
repository = register_settings.create_repository()
Expand All @@ -45,6 +58,11 @@ def register(register_settings: settings.Settings, package: str) -> None:


def main(args: List[str]) -> None:
"""Entry-point of register command.

:param List[str] args:
Arguments for the register command.
"""
parser = argparse.ArgumentParser(
prog="twine register",
description="register operation is not required with PyPI.org",
Expand Down
56 changes: 55 additions & 1 deletion twine/commands/upload.py
@@ -1,3 +1,7 @@
"""Module containing the upload function in twine.

This module uploads the package to the repository.
"""
# Copyright 2013 Donald Stufft
#
# Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -30,6 +34,26 @@
def skip_upload(
response: requests.Response, skip_existing: bool, package: package_file.PackageFile
) -> bool:
"""Skip uploading a package.

Return Boolean type according to the status code that responded by the repository or
the argument passed by the user when trying to upload the package(s).

If ``skip_existing`` is set to ``True``, then return ``False``.
If status code 400, 403, 409 is responded by the repository, return ``True``.

:param requests.Response response:
bhrutledge marked this conversation as resolved.
Show resolved Hide resolved
Get the response from the repository.
:param bool skip_existing:
Specify whether twine should continue uploading files if one
of them already exists. This primarily supports PyPI. Other
package indexes may not be supported.
:param package_file.PackageFile package:
bhrutledge marked this conversation as resolved.
Show resolved Hide resolved
Get the package files.

:return bool:
Determine whether we should skip uploading the package.
"""
bhrutledge marked this conversation as resolved.
Show resolved Hide resolved
if not skip_existing:
return False

Expand Down Expand Up @@ -57,7 +81,6 @@ def skip_upload(
def _make_package(
filename: str, signatures: Dict[str, str], upload_settings: settings.Settings
) -> package_file.PackageFile:
"""Create and sign a package, based off of filename, signatures and settings."""
Copy link
Contributor

Choose a reason for hiding this comment

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

@meowmeowmeowcat Why was this and the docstring for check._check_file deleted? If anything, I'm inclined to require docstrings on private functions and methods, since they often have non-trivial logic.

Copy link
Member Author

@dukecat0 dukecat0 Sep 12, 2021

Choose a reason for hiding this comment

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

I think it's because I just want to pass the flake8 test and make the review easier, so I only add docstrings to public functions first. If you want, I can take some time and add docstrings to private functions. (Or do you prefer adding them by yourself?) :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good instinct to limit the scope, and at this point I'd rather add private docstrings in a separate PR. However, it seems like you deleted these docstrings. Were they causing flake8 to fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

However, it seems like you deleted these docstrings. Were they causing flake8 to fail?

I think they were not causing flake8 to fail. Only missing public docstrings will cause flake8 to fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. With that in mind, I've restored them.

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

signed_name = package.signed_basefilename
Expand All @@ -75,6 +98,32 @@ def _make_package(


def upload(upload_settings: settings.Settings, dists: List[str]) -> None:
"""Upload distributions to repository.

This function will check if the user has passed in pre-signed distributions.
Then, it will check the repository url to verify that we are not using legacy
PyPI. If no error is occurred, it will make package, create repository and
upload distributions.

If ``skip_existing`` is set to ``True`` and the package is uploaded already,
it prints the skipping message to the user and continues uploading distributions.

If we get a redirect, exception :class:`RedirectDetected` is raised.

If ``skip_upload`` is ``True``, it prints the skipping message to the user
and continues uploading distributions.

Then, it will check status code responded by the repository, and generate
a helpful message. After that, it will add the distribution files uploaded to
the ``uploaded_packages``.

Finally, it will show release urls to the user and close the session.

:param settings.Settings upload_settings:
bhrutledge marked this conversation as resolved.
Show resolved Hide resolved
The settings for the upload function.
:param List[str] dists:
Get dists that are going to be uploaded.
"""
dists = commands._find_dists(dists)
# Determine if the user has passed in pre-signed distributions
signatures = {os.path.basename(d): d for d in dists if d.endswith(".asc")}
Expand Down Expand Up @@ -135,6 +184,11 @@ def upload(upload_settings: settings.Settings, dists: List[str]) -> None:


def main(args: List[str]) -> None:
"""Entry-point of upload command.

:param List[str] args:
Arguments for the upload command.
"""
parser = argparse.ArgumentParser(prog="twine upload")
settings.Settings.register_argparse_arguments(parser)
parser.add_argument(
Expand Down