Setup basic logging functionality(existing output) with 1 level of verbosity #670
Conversation
|
@VikramJayanthi17 Thanks for submitting this. I'm aiming to review in the next day or two. |
| import importlib.metadata as importlib_metadata | ||
| from importlib import metadata as importlib_metadata |
There was a problem hiding this comment.
What prompted this change?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@VikramJayanthi17 Thanks for the changes. I took the liberty of pushing one more structural change (described in a comment) to address a gotcha, figuring it'd be easy to revert if necessary. What do you think?
| import importlib.metadata as importlib_metadata | ||
| from importlib import metadata as importlib_metadata |
There was a problem hiding this comment.
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.
| @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) |
There was a problem hiding this comment.
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.
Lines 79 to 81 in 27f029b
Furthermore, the tests were still setting Settings.verbose directly, which wouldn't actually set up logging:
Lines 71 to 74 in 27f029b
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.
| @pytest.mark.parametrize( | ||
| "verbose", [True, False], | ||
| ) | ||
| def test_check_status_code_for_missing_status_code( | ||
| capsys, repo_url, verbose, make_settings | ||
| ): |
There was a problem hiding this comment.
I think this test should have been parameterized when it was written.
|
@bhrutledge That change makes a lot of sense, it looks good to me! Thanks for the help. |
|
@sigmavirus24 Do you have any additional input on this PR? I can't seem to add you as a reviewer. |
|
Unless @sigmavirus24 has any additional comments, I'll merge this tomorrow morning. |
As suggested by @bhrutledge in #669, I created a new branch and have replicated existing
verbosefunctionality(distribution names, signatures,Etc.) usingstdlib. The goal is to use this logging system and potentially add multiple levels of verbosity and consequently more detailed output(as mentioned in #381).