From 39b927eeb31f59d7dfaab188bd3b93c375cb8a88 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 1 Jan 2022 16:10:53 -0500 Subject: [PATCH 01/14] Replace warnings.warn with logger.warning --- tests/test_auth.py | 12 ++++-------- twine/auth.py | 5 ++--- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index cf0fddd7..1050ef00 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -149,21 +149,17 @@ def get_credential(system, username): def test_get_username_runtime_error_suppressed( - entered_username, keyring_no_backends_get_credential, recwarn, config + entered_username, keyring_no_backends_get_credential, caplog, config ): assert auth.Resolver(config, auth.CredentialInput()).username == "entered user" - assert len(recwarn) == 1 - warning = recwarn.pop(UserWarning) - assert "fail!" in str(warning) + assert caplog.messages == ["fail!"] def test_get_password_runtime_error_suppressed( - entered_password, keyring_no_backends, recwarn, config + entered_password, keyring_no_backends, caplog, config ): assert auth.Resolver(config, auth.CredentialInput("user")).password == "entered pw" - assert len(recwarn) == 1 - warning = recwarn.pop(UserWarning) - assert "fail!" in str(warning) + assert caplog.messages == ["fail!"] def test_get_username_return_none(entered_username, monkeypatch, config): diff --git a/twine/auth.py b/twine/auth.py index 514a0d15..a873bf7c 100644 --- a/twine/auth.py +++ b/twine/auth.py @@ -1,7 +1,6 @@ import functools import getpass import logging -import warnings from typing import Callable, Optional, Type, cast import keyring @@ -64,7 +63,7 @@ def get_username_from_keyring(self) -> Optional[str]: # To support keyring prior to 15.2 pass except Exception as exc: - warnings.warn(str(exc)) + logger.warning(str(exc)) return None def get_password_from_keyring(self) -> Optional[str]: @@ -74,7 +73,7 @@ def get_password_from_keyring(self) -> Optional[str]: logger.info("Querying keyring for password") return cast(str, keyring.get_password(system, username)) except Exception as exc: - warnings.warn(str(exc)) + logger.warning(str(exc)) return None def username_from_keyring_or_prompt(self) -> str: From 7a88bc7a056b55890b67206901429b5425e28d6d Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 1 Jan 2022 17:03:30 -0500 Subject: [PATCH 02/14] Use rich for logging --- setup.cfg | 1 + twine/commands/upload.py | 4 ++-- twine/settings.py | 12 ++++++++++-- twine/utils.py | 4 ++-- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/setup.cfg b/setup.cfg index c3ac2f0c..ff2b116c 100644 --- a/setup.cfg +++ b/setup.cfg @@ -46,6 +46,7 @@ install_requires= keyring >= 15.1 rfc3986 >= 1.4.0 colorama >= 0.4.3 + rich include_package_data = True [options.entry_points] diff --git a/twine/commands/upload.py b/twine/commands/upload.py index 45b6f2a1..21d7f831 100644 --- a/twine/commands/upload.py +++ b/twine/commands/upload.py @@ -82,9 +82,9 @@ def _make_package( package.sign(upload_settings.sign_with, upload_settings.identity) file_size = utils.get_file_size(package.filename) - logger.info(f" {package.filename} ({file_size})") + logger.info(f"{package.filename} ({file_size})") if package.gpg_signature: - logger.info(f" Signed with {package.signed_filename}") + logger.info(f"Signed with {package.signed_filename}") return package diff --git a/twine/settings.py b/twine/settings.py index 4f7b96a4..a21c3f29 100644 --- a/twine/settings.py +++ b/twine/settings.py @@ -15,9 +15,11 @@ import argparse import contextlib import logging -import sys from typing import Any, ContextManager, Optional, cast +import rich.highlighter +import rich.logging + from twine import auth from twine import exceptions from twine import repository @@ -151,7 +153,13 @@ def verbose(self, verbose: bool) -> None: """Initialize a logger based on the --verbose option.""" self._verbose = verbose root_logger = logging.getLogger("twine") - root_logger.addHandler(logging.StreamHandler(sys.stdout)) + root_logger.addHandler( + rich.logging.RichHandler( + show_time=False, + show_path=False, + highlighter=rich.highlighter.NullHighlighter(), + ) + ) root_logger.setLevel(logging.INFO if verbose else logging.WARNING) @staticmethod diff --git a/twine/utils.py b/twine/utils.py index 0aa5c75a..a0cd5eab 100644 --- a/twine/utils.py +++ b/twine/utils.py @@ -258,9 +258,9 @@ def get_userpass_value( warning = f"Your {key} contains control characters" if warning: - logger.warning(f" {warning}. Did you enter it correctly?") + logger.warning(f"{warning}. Did you enter it correctly?") logger.warning( - " See https://twine.readthedocs.io/#entering-credentials " + "See https://twine.readthedocs.io/#entering-credentials " "for more information." ) From 424a149e99efb34832a84fdafa7d26333d0665fd Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sun, 2 Jan 2022 05:54:00 -0500 Subject: [PATCH 03/14] Use Rich for top-level errors --- mypy.ini | 4 ---- setup.cfg | 1 - twine/__main__.py | 32 ++++++++++++++++++-------------- twine/settings.py | 10 ---------- 4 files changed, 18 insertions(+), 29 deletions(-) diff --git a/mypy.ini b/mypy.ini index e91099ed..5f2d5780 100644 --- a/mypy.ini +++ b/mypy.ini @@ -18,10 +18,6 @@ warn_return_any = True no_implicit_reexport = True strict_equality = True -[mypy-colorama] -; https://github.com/tartley/colorama/issues/206 -ignore_missing_imports = True - [mypy-pkginfo] ; https://bugs.launchpad.net/pkginfo/+bug/1876591 ignore_missing_imports = True diff --git a/setup.cfg b/setup.cfg index ff2b116c..3073d0dc 100644 --- a/setup.cfg +++ b/setup.cfg @@ -45,7 +45,6 @@ install_requires= importlib_metadata >= 3.6 keyring >= 15.1 rfc3986 >= 1.4.0 - colorama >= 0.4.3 rich include_package_data = True diff --git a/twine/__main__.py b/twine/__main__.py index fd0133e2..099b11c7 100644 --- a/twine/__main__.py +++ b/twine/__main__.py @@ -13,40 +13,44 @@ # See the License for the specific language governing permissions and # limitations under the License. import http +import logging import sys from typing import Any -import colorama import requests +import rich.highlighter +import rich.logging from twine import cli from twine import exceptions def main() -> Any: + root_logger = logging.getLogger("twine") + root_logger.addHandler( + rich.logging.RichHandler( + show_time=False, + show_path=False, + highlighter=rich.highlighter.NullHighlighter(), + ) + ) + try: - result = cli.dispatch(sys.argv[1:]) + error = cli.dispatch(sys.argv[1:]) except requests.HTTPError as exc: + error = True status_code = exc.response.status_code status_phrase = http.HTTPStatus(status_code).phrase - result = ( + root_logger.error( f"{exc.__class__.__name__}: {status_code} {status_phrase} " f"from {exc.response.url}\n" f"{exc.response.reason}" ) except exceptions.TwineException as exc: - result = f"{exc.__class__.__name__}: {exc.args[0]}" - - return _format_error(result) if isinstance(result, str) else result - - -def _format_error(message: str) -> str: - pre_style, post_style = "", "" - if not cli.args.no_color: - colorama.init() - pre_style, post_style = colorama.Fore.RED, colorama.Style.RESET_ALL + error = True + root_logger.error(f"{exc.__class__.__name__}: {exc.args[0]}") - return f"{pre_style}{message}{post_style}" + return error if __name__ == "__main__": diff --git a/twine/settings.py b/twine/settings.py index a21c3f29..7c89e771 100644 --- a/twine/settings.py +++ b/twine/settings.py @@ -17,9 +17,6 @@ import logging from typing import Any, ContextManager, Optional, cast -import rich.highlighter -import rich.logging - from twine import auth from twine import exceptions from twine import repository @@ -153,13 +150,6 @@ def verbose(self, verbose: bool) -> None: """Initialize a logger based on the --verbose option.""" self._verbose = verbose root_logger = logging.getLogger("twine") - root_logger.addHandler( - rich.logging.RichHandler( - show_time=False, - show_path=False, - highlighter=rich.highlighter.NullHighlighter(), - ) - ) root_logger.setLevel(logging.INFO if verbose else logging.WARNING) @staticmethod From ed388e374fd4dbf396e07f8b615c5c6b4366dfab Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sun, 2 Jan 2022 06:31:56 -0500 Subject: [PATCH 04/14] Update log level colors --- twine/__main__.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/twine/__main__.py b/twine/__main__.py index 099b11c7..cc78b0ea 100644 --- a/twine/__main__.py +++ b/twine/__main__.py @@ -18,8 +18,10 @@ from typing import Any import requests +import rich.console import rich.highlighter import rich.logging +import rich.theme from twine import cli from twine import exceptions @@ -29,6 +31,17 @@ def main() -> Any: root_logger = logging.getLogger("twine") root_logger.addHandler( rich.logging.RichHandler( + console=rich.console.Console( + theme=rich.theme.Theme( + { + "logging.level.debug": "green", + "logging.level.info": "blue", + "logging.level.warning": "yellow", + "logging.level.error": "red", + "logging.level.critical": "reverse red", + } + ) + ), show_time=False, show_path=False, highlighter=rich.highlighter.NullHighlighter(), From 4d34ed1f29045abedc3b1e099e2929bbf497f250 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sun, 23 Jan 2022 06:39:50 -0500 Subject: [PATCH 05/14] Use caplog in tests instead of capsys. When using Rich, capsys results in wrapped output, which would be very hard to make assertions on. --- tests/test_auth.py | 2 +- tests/test_main.py | 38 +++++++++++++++++++++++++++++------- tests/test_settings.py | 9 ++++----- tests/test_upload.py | 44 ++++++++++++++++++++++++------------------ tests/test_utils.py | 13 ++++++++----- 5 files changed, 69 insertions(+), 37 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 1050ef00..60d20965 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -223,4 +223,4 @@ def test_warns_for_empty_password( assert auth.Resolver(config, auth.CredentialInput()).password == password - assert caplog.messages[0].startswith(f" {warning}") + assert caplog.messages[0].startswith(warning) diff --git a/tests/test_main.py b/tests/test_main.py index 9fa53dcf..58c06770 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -12,18 +12,42 @@ import sys -import colorama +import pytest from twine import __main__ as dunder_main -def test_exception_handling(monkeypatch): +def test_exception_handling(monkeypatch, capsys): monkeypatch.setattr(sys, "argv", ["twine", "upload", "missing.whl"]) - message = "InvalidDistribution: Cannot find file (or expand pattern): 'missing.whl'" - assert dunder_main.main() == colorama.Fore.RED + message + colorama.Style.RESET_ALL + error = dunder_main.main() + assert error -def test_no_color_exception(monkeypatch): + captured = capsys.readouterr() + + # Removing trailing whitespace on lines wrapped by Rich; trying to test it was ugly. + # TODO: Assert color + assert [line.rstrip() for line in captured.out.splitlines()] == [ + "ERROR InvalidDistribution: Cannot find file (or expand pattern):", + " 'missing.whl'", + ] + + +@pytest.mark.xfail(reason="capsys isn't reset, resulting in duplicate lines") +def test_no_color_exception(monkeypatch, capsys): monkeypatch.setattr(sys, "argv", ["twine", "--no-color", "upload", "missing.whl"]) - message = "InvalidDistribution: Cannot find file (or expand pattern): 'missing.whl'" - assert dunder_main.main() == message + + error = dunder_main.main() + assert error + + captured = capsys.readouterr() + + # Removing trailing whitespace on lines wrapped by Rich; trying to test it was ugly. + # TODO: Assert no color + assert [line.rstrip() for line in captured.out.splitlines()] == [ + "ERROR InvalidDistribution: Cannot find file (or expand pattern):", + " 'missing.whl'", + ] + + +# TODO: Test verbose output formatting diff --git a/tests/test_settings.py b/tests/test_settings.py index d7c4cdfb..1025eae7 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -17,6 +17,7 @@ import pytest +from twine import __main__ as dunder_main from twine import exceptions from twine import settings @@ -66,16 +67,14 @@ def test_setup_logging(verbose, log_level): "verbose", [True, False], ) -def test_print_config_path_if_verbose(config_file, capsys, make_settings, verbose): +def test_print_config_path_if_verbose(config_file, caplog, make_settings, verbose): """Print the location of the .pypirc config used by the user.""" make_settings(verbose=verbose) - captured = capsys.readouterr() - if verbose: - assert captured.out == f"Using configuration from {config_file}\n" + assert caplog.messages == [f"Using configuration from {config_file}"] else: - assert captured.out == "" + assert caplog.messages == [] def test_identity_requires_sign(): diff --git a/tests/test_upload.py b/tests/test_upload.py index e6ff45e1..6a13d899 100644 --- a/tests/test_upload.py +++ b/tests/test_upload.py @@ -54,7 +54,7 @@ def upload_settings(make_settings, stub_repository): return upload_settings -def test_make_package_pre_signed_dist(upload_settings, capsys): +def test_make_package_pre_signed_dist(upload_settings, caplog): """Create a PackageFile and print path, size, and user-provided signature.""" filename = helpers.WHEEL_FIXTURE expected_size = "15.4 KB" @@ -69,12 +69,13 @@ def test_make_package_pre_signed_dist(upload_settings, capsys): assert package.filename == filename assert package.gpg_signature is not None - captured = capsys.readouterr() - assert captured.out.count(f"{filename} ({expected_size})") == 1 - assert captured.out.count(f"Signed with {signed_filename}") == 1 + assert caplog.messages == [ + f"{filename} ({expected_size})", + f"Signed with {signed_filename}", + ] -def test_make_package_unsigned_dist(upload_settings, monkeypatch, capsys): +def test_make_package_unsigned_dist(upload_settings, monkeypatch, caplog): """Create a PackageFile and print path, size, and Twine-generated signature.""" filename = helpers.NEW_WHEEL_FIXTURE expected_size = "21.9 KB" @@ -93,9 +94,10 @@ def stub_sign(package, *_): assert package.filename == filename assert package.gpg_signature is not None - captured = capsys.readouterr() - assert captured.out.count(f"{filename} ({expected_size})") == 1 - assert captured.out.count(f"Signed with {package.signed_filename}") == 1 + assert caplog.messages == [ + f"{filename} ({expected_size})", + f"Signed with {package.signed_filename}", + ] def test_successs_prints_release_urls(upload_settings, stub_repository, capsys): @@ -118,13 +120,13 @@ def test_successs_prints_release_urls(upload_settings, stub_repository, capsys): assert captured.out.count(NEW_RELEASE_URL) == 1 -def test_print_packages_if_verbose(upload_settings, capsys): +def test_print_packages_if_verbose(upload_settings, caplog): """Print the path and file size of each distribution attempting to be uploaded.""" dists_to_upload = { helpers.WHEEL_FIXTURE: "15.4 KB", + helpers.NEW_WHEEL_FIXTURE: "21.9 KB", helpers.SDIST_FIXTURE: "20.8 KB", helpers.NEW_SDIST_FIXTURE: "26.1 KB", - helpers.NEW_WHEEL_FIXTURE: "21.9 KB", } upload_settings.verbose = True @@ -132,10 +134,9 @@ def test_print_packages_if_verbose(upload_settings, capsys): result = upload.upload(upload_settings, dists_to_upload.keys()) assert result is None - captured = capsys.readouterr() - - for filename, size in dists_to_upload.items(): - assert captured.out.count(f"{filename} ({size})") == 1 + assert caplog.messages == [ + f"{filename} ({size})" for filename, size in dists_to_upload.items() + ] def test_success_with_pre_signed_distribution(upload_settings, stub_repository): @@ -181,7 +182,9 @@ def test_success_when_gpg_is_run(upload_settings, stub_repository, monkeypatch): @pytest.mark.parametrize("verbose", [False, True]) -def test_exception_for_http_status(verbose, upload_settings, stub_response, capsys): +def test_exception_for_http_status( + verbose, upload_settings, stub_response, capsys, caplog +): upload_settings.verbose = verbose stub_response.is_redirect = False @@ -196,11 +199,14 @@ def test_exception_for_http_status(verbose, upload_settings, stub_response, caps assert RELEASE_URL not in captured.out if verbose: - assert stub_response.text in captured.out - assert "--verbose" not in captured.out + assert caplog.messages == [ + f"{helpers.WHEEL_FIXTURE} (15.4 KB)", + f"Content received from server:\n{stub_response.text}", + ] else: - assert stub_response.text not in captured.out - assert "--verbose" in captured.out + assert caplog.messages == [ + "Error during upload. Retry with the --verbose option for more details." + ] def test_get_config_old_format(make_settings, config_file): diff --git a/tests/test_utils.py b/tests/test_utils.py index 07965d54..a592f3bb 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -265,7 +265,7 @@ def test_check_status_code_for_deprecated_pypi_url(repo_url): [True, False], ) def test_check_status_code_for_missing_status_code( - capsys, repo_url, verbose, make_settings + caplog, repo_url, verbose, make_settings, config_file ): """Print HTTP errors based on verbosity level.""" response = pretend.stub( @@ -280,12 +280,15 @@ def test_check_status_code_for_missing_status_code( with pytest.raises(requests.HTTPError): utils.check_status_code(response, verbose) - captured = capsys.readouterr() - if verbose: - assert captured.out.count("Content received from server:\nForbidden\n") == 1 + assert caplog.messages == [ + f"Using configuration from {config_file}", + f"Content received from server:\n{response.text}", + ] else: - assert captured.out.count("--verbose option") == 1 + assert caplog.messages == [ + "Error during upload. Retry with the --verbose option for more details." + ] @pytest.mark.parametrize( From b7b104056075988382f7f37bdf1f495713310df5 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sun, 23 Jan 2022 10:14:47 -0500 Subject: [PATCH 06/14] Test color output --- tests/test_main.py | 7 ++++--- twine/__main__.py | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/test_main.py b/tests/test_main.py index 58c06770..d9e90fb9 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -25,10 +25,11 @@ def test_exception_handling(monkeypatch, capsys): captured = capsys.readouterr() - # Removing trailing whitespace on lines wrapped by Rich; trying to test it was ugly. - # TODO: Assert color + # Hard-coding control characters for red text; couldn't find a succint alternative. + # Removing trailing whitespace on wrapped lines; trying to test it was ugly. + level = "\x1b[31mERROR \x1b[0m" assert [line.rstrip() for line in captured.out.splitlines()] == [ - "ERROR InvalidDistribution: Cannot find file (or expand pattern):", + f"{level} InvalidDistribution: Cannot find file (or expand pattern):", " 'missing.whl'", ] diff --git a/twine/__main__.py b/twine/__main__.py index cc78b0ea..3651ae40 100644 --- a/twine/__main__.py +++ b/twine/__main__.py @@ -40,7 +40,8 @@ def main() -> Any: "logging.level.error": "red", "logging.level.critical": "reverse red", } - ) + ), + force_terminal=True, ), show_time=False, show_path=False, From 5a3006ec92debb0ca819d50ff0addfeb38ce1c7e Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sun, 23 Jan 2022 11:12:57 -0500 Subject: [PATCH 07/14] Support --no-color --- tests/test_main.py | 3 +-- twine/__main__.py | 26 +++----------------------- twine/cli.py | 42 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 26 deletions(-) diff --git a/tests/test_main.py b/tests/test_main.py index d9e90fb9..b3df87e0 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -43,8 +43,7 @@ def test_no_color_exception(monkeypatch, capsys): captured = capsys.readouterr() - # Removing trailing whitespace on lines wrapped by Rich; trying to test it was ugly. - # TODO: Assert no color + # Removing trailing whitespace on wrapped lines; trying to test it was ugly. assert [line.rstrip() for line in captured.out.splitlines()] == [ "ERROR InvalidDistribution: Cannot find file (or expand pattern):", " 'missing.whl'", diff --git a/twine/__main__.py b/twine/__main__.py index 3651ae40..1173eb63 100644 --- a/twine/__main__.py +++ b/twine/__main__.py @@ -18,36 +18,16 @@ from typing import Any import requests -import rich.console -import rich.highlighter -import rich.logging -import rich.theme from twine import cli from twine import exceptions def main() -> Any: + # Ensure that all log messages are displayed. + # Color will be configured during cli.dispatch() after argparse. root_logger = logging.getLogger("twine") - root_logger.addHandler( - rich.logging.RichHandler( - console=rich.console.Console( - theme=rich.theme.Theme( - { - "logging.level.debug": "green", - "logging.level.info": "blue", - "logging.level.warning": "yellow", - "logging.level.error": "red", - "logging.level.critical": "reverse red", - } - ), - force_terminal=True, - ), - show_time=False, - show_path=False, - highlighter=rich.highlighter.NullHighlighter(), - ) - ) + root_logger.addHandler(logging.StreamHandler(sys.stdout)) try: error = cli.dispatch(sys.argv[1:]) diff --git a/twine/cli.py b/twine/cli.py index f65bc035..9b7e9a06 100644 --- a/twine/cli.py +++ b/twine/cli.py @@ -12,9 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. import argparse +import logging from typing import Any, List, Tuple import importlib_metadata +import rich.console +import rich.highlighter +import rich.logging +import rich.theme from packaging import requirements import twine @@ -34,10 +39,44 @@ def dep_versions() -> str: ) +def configure_logging() -> None: + root_logger = logging.getLogger("twine") + + # Overwrite basic configuration in main() + # TODO: Use dictConfig() instead? + for handler in root_logger.handlers: + root_logger.removeHandler(handler) + + root_logger.addHandler( + rich.logging.RichHandler( + # TODO: Maybe make console a module attribute to facilitate testing and + # using Rich's other functionality. + console=rich.console.Console( + # TODO: Set this if FORCE_COLOR or PY_COLORS in os.environ + force_terminal=True, + no_color=getattr(args, "no_color", False), + theme=rich.theme.Theme( + { + "logging.level.debug": "green", + "logging.level.info": "blue", + "logging.level.warning": "yellow", + "logging.level.error": "red", + "logging.level.critical": "reverse red", + } + ), + ), + show_time=False, + show_path=False, + highlighter=rich.highlighter.NullHighlighter(), + ) + ) + + def dispatch(argv: List[str]) -> Any: registered_commands = importlib_metadata.entry_points( group="twine.registered_commands" ) + parser = argparse.ArgumentParser(prog="twine") parser.add_argument( "--version", @@ -60,9 +99,10 @@ def dispatch(argv: List[str]) -> Any: help=argparse.SUPPRESS, nargs=argparse.REMAINDER, ) - parser.parse_args(argv, namespace=args) + configure_logging() + main = registered_commands[args.command].load() return main(args.args) From ab165c31ae9d336712dd3a0cb4ffdf17d058d91e Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sun, 23 Jan 2022 12:03:54 -0500 Subject: [PATCH 08/14] Fix --no-color test --- tests/test_main.py | 3 --- twine/__main__.py | 12 ++++----- twine/cli.py | 66 ++++++++++++++++++++++++++-------------------- 3 files changed, 43 insertions(+), 38 deletions(-) diff --git a/tests/test_main.py b/tests/test_main.py index b3df87e0..1a7b2f5a 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -12,8 +12,6 @@ import sys -import pytest - from twine import __main__ as dunder_main @@ -34,7 +32,6 @@ def test_exception_handling(monkeypatch, capsys): ] -@pytest.mark.xfail(reason="capsys isn't reset, resulting in duplicate lines") def test_no_color_exception(monkeypatch, capsys): monkeypatch.setattr(sys, "argv", ["twine", "--no-color", "upload", "missing.whl"]) diff --git a/twine/__main__.py b/twine/__main__.py index 1173eb63..d13a2e87 100644 --- a/twine/__main__.py +++ b/twine/__main__.py @@ -22,12 +22,12 @@ from twine import cli from twine import exceptions +logger = logging.getLogger(__name__) + def main() -> Any: - # Ensure that all log messages are displayed. - # Color will be configured during cli.dispatch() after argparse. - root_logger = logging.getLogger("twine") - root_logger.addHandler(logging.StreamHandler(sys.stdout)) + # Ensure that all errors are logged, even before argparse + cli.configure_logging() try: error = cli.dispatch(sys.argv[1:]) @@ -35,14 +35,14 @@ def main() -> Any: error = True status_code = exc.response.status_code status_phrase = http.HTTPStatus(status_code).phrase - root_logger.error( + logger.error( f"{exc.__class__.__name__}: {status_code} {status_phrase} " f"from {exc.response.url}\n" f"{exc.response.reason}" ) except exceptions.TwineException as exc: error = True - root_logger.error(f"{exc.__class__.__name__}: {exc.args[0]}") + logger.error(f"{exc.__class__.__name__}: {exc.args[0]}") return error diff --git a/twine/cli.py b/twine/cli.py index 9b7e9a06..801a9a37 100644 --- a/twine/cli.py +++ b/twine/cli.py @@ -26,45 +26,38 @@ args = argparse.Namespace() - -def list_dependencies_and_versions() -> List[Tuple[str, str]]: - requires = importlib_metadata.requires("twine") # type: ignore[no-untyped-call] # python/importlib_metadata#288 # noqa: E501 - deps = [requirements.Requirement(r).name for r in requires] - return [(dep, importlib_metadata.version(dep)) for dep in deps] # type: ignore[no-untyped-call] # python/importlib_metadata#288 # noqa: E501 - - -def dep_versions() -> str: - return ", ".join( - "{}: {}".format(*dependency) for dependency in list_dependencies_and_versions() - ) +# This module attribute facilitates project-wide configuration and usage of Rich. +# https://rich.readthedocs.io/en/latest/console.html +console = rich.console.Console( + # Setting force_terminal makes testing easier by ensuring color codes. + # This could be based on FORCE_COLORS or PY_COLORS in os.environ, but Rich doesn't + # support that (https://github.com/Textualize/rich/issues/343), and since this is + # a module attribute, os.environ would be read on import, which complicates testing. + # no_color is set in dispatch() after argparse. + force_terminal=True, + theme=rich.theme.Theme( + { + "logging.level.debug": "green", + "logging.level.info": "blue", + "logging.level.warning": "yellow", + "logging.level.error": "red", + "logging.level.critical": "reverse red", + } + ), +) def configure_logging() -> None: root_logger = logging.getLogger("twine") - # Overwrite basic configuration in main() + # This prevents failures test_main.py due to capsys not being cleared. # TODO: Use dictConfig() instead? for handler in root_logger.handlers: root_logger.removeHandler(handler) root_logger.addHandler( rich.logging.RichHandler( - # TODO: Maybe make console a module attribute to facilitate testing and - # using Rich's other functionality. - console=rich.console.Console( - # TODO: Set this if FORCE_COLOR or PY_COLORS in os.environ - force_terminal=True, - no_color=getattr(args, "no_color", False), - theme=rich.theme.Theme( - { - "logging.level.debug": "green", - "logging.level.info": "blue", - "logging.level.warning": "yellow", - "logging.level.error": "red", - "logging.level.critical": "reverse red", - } - ), - ), + console=console, show_time=False, show_path=False, highlighter=rich.highlighter.NullHighlighter(), @@ -72,6 +65,18 @@ def configure_logging() -> None: ) +def list_dependencies_and_versions() -> List[Tuple[str, str]]: + requires = importlib_metadata.requires("twine") # type: ignore[no-untyped-call] # python/importlib_metadata#288 # noqa: E501 + deps = [requirements.Requirement(r).name for r in requires] + return [(dep, importlib_metadata.version(dep)) for dep in deps] # type: ignore[no-untyped-call] # python/importlib_metadata#288 # noqa: E501 + + +def dep_versions() -> str: + return ", ".join( + "{}: {}".format(*dependency) for dependency in list_dependencies_and_versions() + ) + + def dispatch(argv: List[str]) -> Any: registered_commands = importlib_metadata.entry_points( group="twine.registered_commands" @@ -101,7 +106,10 @@ def dispatch(argv: List[str]) -> Any: ) parser.parse_args(argv, namespace=args) - configure_logging() + # HACK: This attribute isn't documented, but this is an expedient way to alter the + # Rich configuration after argparse, while allowing logging to be configured on + # startup, ensuring all errors are displayed. + console.no_color = args.no_color main = registered_commands[args.command].load() From 1f254730fcf866171b3fdc57f377aab338c53fd2 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sun, 23 Jan 2022 14:34:50 -0500 Subject: [PATCH 09/14] Use dictConfig --- twine/cli.py | 36 +++++++++++++++++++++--------------- twine/settings.py | 4 ++-- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/twine/cli.py b/twine/cli.py index 801a9a37..dba77713 100644 --- a/twine/cli.py +++ b/twine/cli.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import argparse -import logging +import logging.config from typing import Any, List, Tuple import importlib_metadata @@ -48,20 +48,26 @@ def configure_logging() -> None: - root_logger = logging.getLogger("twine") - - # This prevents failures test_main.py due to capsys not being cleared. - # TODO: Use dictConfig() instead? - for handler in root_logger.handlers: - root_logger.removeHandler(handler) - - root_logger.addHandler( - rich.logging.RichHandler( - console=console, - show_time=False, - show_path=False, - highlighter=rich.highlighter.NullHighlighter(), - ) + # Using dictConfig to override existing loggers, which prevents failures in + # test_main.py due to capsys not being cleared. + logging.config.dictConfig( + { + "version": 1, + "handlers": { + "console": { + "class": "rich.logging.RichHandler", + "console": console, + "show_time": False, + "show_path": False, + "highlighter": rich.highlighter.NullHighlighter(), + } + }, + "loggers": { + "twine": { + "handlers": ["console"], + }, + }, + } ) diff --git a/twine/settings.py b/twine/settings.py index 7c89e771..7ea5d4b1 100644 --- a/twine/settings.py +++ b/twine/settings.py @@ -149,8 +149,8 @@ def verbose(self) -> bool: def verbose(self, verbose: bool) -> None: """Initialize a logger based on the --verbose option.""" self._verbose = verbose - root_logger = logging.getLogger("twine") - root_logger.setLevel(logging.INFO if verbose else logging.WARNING) + twine_logger = logging.getLogger("twine") + twine_logger.setLevel(logging.INFO if verbose else logging.WARNING) @staticmethod def register_argparse_arguments(parser: argparse.ArgumentParser) -> None: From 2dc2058a329babbb26e2c976454c8b86148c1c0b Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sun, 23 Jan 2022 15:04:56 -0500 Subject: [PATCH 10/14] Fix unused import --- tests/test_settings.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_settings.py b/tests/test_settings.py index 1025eae7..57e980c5 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -17,7 +17,6 @@ import pytest -from twine import __main__ as dunder_main from twine import exceptions from twine import settings From 0a3eb469c4eb6845714457d6bd7495912055e2a1 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sun, 23 Jan 2022 16:36:01 -0500 Subject: [PATCH 11/14] Avoid keyring backend warning --- tests/test_auth.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_auth.py b/tests/test_auth.py index 60d20965..47c6d1c5 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -219,6 +219,9 @@ def test_warns_for_empty_password( config, caplog, ): + # Avoiding additional warning "No recommended backend was available" + monkeypatch.setattr(auth.keyring, "get_password", lambda system, user: None) + monkeypatch.setattr(getpass, "getpass", lambda prompt: password) assert auth.Resolver(config, auth.CredentialInput()).password == password From c6808e022515cdb6055d78ae312e560758f57f92 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sun, 23 Jan 2022 16:55:43 -0500 Subject: [PATCH 12/14] Add coverage for HTTP error logging --- tests/test_main.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/test_main.py b/tests/test_main.py index 1a7b2f5a..3151da0c 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -12,7 +12,11 @@ import sys +import pretend +import requests + from twine import __main__ as dunder_main +from twine.commands import upload def test_exception_handling(monkeypatch, capsys): @@ -32,6 +36,36 @@ def test_exception_handling(monkeypatch, capsys): ] +def test_http_exception_handling(monkeypatch, capsys): + monkeypatch.setattr(sys, "argv", ["twine", "upload", "test.whl"]) + monkeypatch.setattr( + upload, + "upload", + pretend.raiser( + requests.HTTPError( + response=pretend.stub( + url="https://example.org", + status_code=400, + reason="Error reason", + ) + ) + ), + ) + + error = dunder_main.main() + assert error + + captured = capsys.readouterr() + + # Hard-coding control characters for red text; couldn't find a succint alternative. + # Removing trailing whitespace on wrapped lines; trying to test it was ugly. + level = "\x1b[31mERROR \x1b[0m" + assert [line.rstrip() for line in captured.out.splitlines()] == [ + f"{level} HTTPError: 400 Bad Request from https://example.org", + " Error reason", + ] + + def test_no_color_exception(monkeypatch, capsys): monkeypatch.setattr(sys, "argv", ["twine", "--no-color", "upload", "missing.whl"]) From 969759a6589264a7e6cae3b8f54f34274d7708a1 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Mon, 24 Jan 2022 06:17:12 -0500 Subject: [PATCH 13/14] Use reconfigure instead of module attribute --- twine/__main__.py | 2 +- twine/cli.py | 48 ++++++++++++++++++++++------------------------- 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/twine/__main__.py b/twine/__main__.py index d13a2e87..493ffb1c 100644 --- a/twine/__main__.py +++ b/twine/__main__.py @@ -27,7 +27,7 @@ def main() -> Any: # Ensure that all errors are logged, even before argparse - cli.configure_logging() + cli.configure_output() try: error = cli.dispatch(sys.argv[1:]) diff --git a/twine/cli.py b/twine/cli.py index dba77713..d137e850 100644 --- a/twine/cli.py +++ b/twine/cli.py @@ -16,7 +16,7 @@ from typing import Any, List, Tuple import importlib_metadata -import rich.console +import rich import rich.highlighter import rich.logging import rich.theme @@ -26,28 +26,28 @@ args = argparse.Namespace() -# This module attribute facilitates project-wide configuration and usage of Rich. -# https://rich.readthedocs.io/en/latest/console.html -console = rich.console.Console( - # Setting force_terminal makes testing easier by ensuring color codes. - # This could be based on FORCE_COLORS or PY_COLORS in os.environ, but Rich doesn't - # support that (https://github.com/Textualize/rich/issues/343), and since this is - # a module attribute, os.environ would be read on import, which complicates testing. - # no_color is set in dispatch() after argparse. - force_terminal=True, - theme=rich.theme.Theme( - { - "logging.level.debug": "green", - "logging.level.info": "blue", - "logging.level.warning": "yellow", - "logging.level.error": "red", - "logging.level.critical": "reverse red", - } - ), -) +def configure_output() -> None: + # Configure the global Console, available via rich.get_console(). + # https://rich.readthedocs.io/en/latest/reference/init.html + # https://rich.readthedocs.io/en/latest/console.html + rich.reconfigure( + # Setting force_terminal makes testing easier by ensuring color codes. This + # could be based on FORCE_COLORS or PY_COLORS in os.environ, since Rich + # doesn't support that (https://github.com/Textualize/rich/issues/343). + force_terminal=True, + no_color=getattr(args, "no_color", False), + theme=rich.theme.Theme( + { + "logging.level.debug": "green", + "logging.level.info": "blue", + "logging.level.warning": "yellow", + "logging.level.error": "red", + "logging.level.critical": "reverse red", + } + ), + ) -def configure_logging() -> None: # Using dictConfig to override existing loggers, which prevents failures in # test_main.py due to capsys not being cleared. logging.config.dictConfig( @@ -56,7 +56,6 @@ def configure_logging() -> None: "handlers": { "console": { "class": "rich.logging.RichHandler", - "console": console, "show_time": False, "show_path": False, "highlighter": rich.highlighter.NullHighlighter(), @@ -112,10 +111,7 @@ def dispatch(argv: List[str]) -> Any: ) parser.parse_args(argv, namespace=args) - # HACK: This attribute isn't documented, but this is an expedient way to alter the - # Rich configuration after argparse, while allowing logging to be configured on - # startup, ensuring all errors are displayed. - console.no_color = args.no_color + configure_output() main = registered_commands[args.command].load() From 2958b7b71506ae3706795f24b10f6d1b9fa4f0e2 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 5 Feb 2022 05:49:50 -0500 Subject: [PATCH 14/14] Add changelog entry --- changelog/818.feature.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/818.feature.rst diff --git a/changelog/818.feature.rst b/changelog/818.feature.rst new file mode 100644 index 00000000..0714ba28 --- /dev/null +++ b/changelog/818.feature.rst @@ -0,0 +1 @@ +Use Rich to add color to ``upload`` logging output.