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

Setup basic logging functionality(existing output) with 1 level of verbosity #670

Merged
merged 4 commits into from Jul 15, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions tests/test_settings.py
Expand Up @@ -13,6 +13,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import argparse
import logging
import os.path
import textwrap

Expand Down Expand Up @@ -55,6 +56,18 @@ def test_settings_transforms_repository_config(tmpdir):
assert s.disable_progress_bar is False


@pytest.mark.parametrize(
"verbose, log_level", [(True, logging.INFO), (False, logging.WARNING)]
)
def test_setup_logging(verbose, log_level):
"""Set log level based on verbose field."""
settings.Settings(verbose=verbose)

logger = logging.getLogger("twine")

assert logger.level == log_level


def test_identity_requires_sign():
"""Raise an exception when user provides identity but doesn't require sigining."""
with pytest.raises(exceptions.InvalidSigningConfiguration):
Expand Down
22 changes: 13 additions & 9 deletions tests/test_utils.py
Expand Up @@ -273,7 +273,12 @@ def test_check_status_code_for_deprecated_pypi_url(repo_url):
@pytest.mark.parametrize(
"repo_url", ["https://pypi.python.org", "https://testpypi.python.org"],
)
def test_check_status_code_for_missing_status_code(capsys, repo_url):
@pytest.mark.parametrize(
"verbose", [True, False],
)
def test_check_status_code_for_missing_status_code(
capsys, repo_url, verbose, make_settings
):
Comment on lines +276 to +281
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test should have been parameterized when it was written.

"""Print HTTP errors based on verbosity level."""
response = pretend.stub(
status_code=403,
Expand All @@ -282,18 +287,17 @@ def test_check_status_code_for_missing_status_code(capsys, repo_url):
text="Forbidden",
)

with pytest.raises(requests.HTTPError):
utils.check_status_code(response, True)

# Different messages are printed based on the verbose level
captured = capsys.readouterr()
assert captured.out == "Content received from server:\nForbidden\n"
make_settings(verbose=verbose)

with pytest.raises(requests.HTTPError):
utils.check_status_code(response, False)
utils.check_status_code(response, verbose)

captured = capsys.readouterr()
assert captured.out == "NOTE: Try --verbose to see response content.\n"

if verbose:
assert captured.out == "Content received from server:\nForbidden\n"
else:
assert captured.out == "NOTE: Try --verbose to see response content.\n"


@pytest.mark.parametrize(
Expand Down
2 changes: 1 addition & 1 deletion twine/__init__.py
Expand Up @@ -27,7 +27,7 @@
import sys

if sys.version_info[:2] >= (3, 8):
import importlib.metadata as importlib_metadata
from importlib import metadata as importlib_metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

What prompted this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tox -e format actually did this automatically. Will change it back.

Edit:

It fails to pass the linting test if I change this back. Any advice on how to proceed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right, isort just had a major version upgrade. I restored this in 8eabc53. I'll plan to followup up with some isort tweaking in a separate PR.

else:
import importlib_metadata

Expand Down
12 changes: 7 additions & 5 deletions twine/commands/upload.py
Expand Up @@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import argparse
import logging
import os.path
from typing import Dict
from typing import List
Expand All @@ -25,6 +26,8 @@
from twine import settings
from twine import utils

logger = logging.getLogger(__name__)


def skip_upload(
response: requests.Response, skip_existing: bool, package: package_file.PackageFile
Expand Down Expand Up @@ -63,11 +66,10 @@ def _make_package(
elif upload_settings.sign:
package.sign(upload_settings.sign_with, upload_settings.identity)

if upload_settings.verbose:
file_size = utils.get_file_size(package.filename)
print(f" {package.filename} ({file_size})")
if package.gpg_signature:
print(f" Signed with {package.signed_filename}")
file_size = utils.get_file_size(package.filename)
logger.info(f" {package.filename} ({file_size})")
if package.gpg_signature:
logger.info(f" Signed with {package.signed_filename}")

return package

Expand Down
14 changes: 14 additions & 0 deletions twine/settings.py
Expand Up @@ -13,6 +13,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import argparse
import logging
import sys
from typing import Any
from typing import Optional
from typing import cast
Expand Down Expand Up @@ -148,6 +150,18 @@ def password(self) -> Optional[str]:
# Workaround for https://github.com/python/mypy/issues/5858
return cast(Optional[str], self.auth.password)

@property
def verbose(self) -> bool:
return self._verbose

@verbose.setter
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.setLevel(logging.INFO if verbose else logging.WARNING)
Comment on lines +153 to +163
Copy link
Contributor

@bhrutledge bhrutledge Jul 9, 2020

Choose a reason for hiding this comment

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

Seeing the switch from capsys to caplog in test_upload.py made me realize that we weren't actually testing that verbose messages were printed to stdout.

captured = caplog.text
assert captured.count(f"{filename} ({expected_size})") == 1
assert captured.count(f"Signed with {signed_filename}") == 1

Furthermore, the tests were still setting Settings.verbose directly, which wouldn't actually set up logging:

upload_settings.sign = True
upload_settings.verbose = True
package = upload._make_package(filename, signatures, upload_settings)

To keep the existing API, avoid rewriting all of the upload tests, and avoid similar gotchas in the future, I made Settings.verbose a @property and moved the logging setup there in 1fd2f58.


@staticmethod
def register_argparse_arguments(parser: argparse.ArgumentParser) -> None:
"""Register the arguments for argparse."""
Expand Down
10 changes: 6 additions & 4 deletions twine/utils.py
Expand Up @@ -15,6 +15,7 @@
import collections
import configparser
import functools
import logging
import os
import os.path
from typing import Any
Expand Down Expand Up @@ -45,6 +46,8 @@
# get_userpass_value.
RepositoryConfig = Dict[str, Optional[str]]

logger = logging.getLogger(__name__)


def get_config(path: str = "~/.pypirc") -> Dict[str, RepositoryConfig]:
# even if the config file does not exist, set up the parser
Expand Down Expand Up @@ -200,10 +203,9 @@ def check_status_code(response: requests.Response, verbose: bool) -> None:
response.raise_for_status()
except requests.HTTPError as err:
if response.text:
if verbose:
print("Content received from server:\n{}".format(response.text))
else:
print("NOTE: Try --verbose to see response content.")
logger.info("Content received from server:\n{}".format(response.text))
if not verbose:
logger.warning("NOTE: Try --verbose to see response content.")
raise err


Expand Down