Log where upload configuration is being loaded from #674
Conversation
|
@VikramJayanthi17 Thanks for this. I should be able to review it in the next day or two. But first, we'll need to get rid of those extra commits, which are a byproduct of me doing a squash & merge on #670. You could do that by:
Also, in general, I think it's best to have a distinct branch for each PR. |
| @@ -64,6 +64,8 @@ def get_config(path: str = "~/.pypirc") -> Dict[str, RepositoryConfig]: | |||
| # Expand user strings in the path | |||
| path = os.path.expanduser(path) | |||
|
|
|||
| logger.info("Config Location: " + path) | |||
There was a problem hiding this comment.
In the last PR we were using fstrings. Here we're concatenating strings. Why don't we just let the stdlib log formatter format strings for us appropriately and why can't we be consistent?
There was a problem hiding this comment.
thought this discussion could help: pylint-dev/pylint#1788
pylint raises a warning on string interpolation during logging, the argument is it is `it is inefficient to compute the string for a log message that won't be logged.'
but f-string is more readable.
There was a problem hiding this comment.
@snjypl that contributes nothing to the fact that there's no consistency in this project at the current moment and we should trend towards something consistent.
There was a problem hiding this comment.
@sigmavirus24 Do you think that we should convert log messages to follow stdlib's log formatter format strings or stick with f-strings? Sorry about the inconsistency, I will keep that in mind moving forward. Thanks for the help.
There was a problem hiding this comment.
Personally I prefer letting the logger do the interpolation, but my primary concern is consistency
There was a problem hiding this comment.
I'm going to stick with f-strings for now then because that's what Twine uses for other output(such as the skip message).
VikramJayanthi17
force-pushed
the
print-to-log
branch
from
df37011
to
5758261
Compare
As mentioned in #381, we now log where the upload configuration is being loaded from(see attached image).
Additionally, I have the rest of the functionality, other than logging credential setting behavior, completed. @bhrutledge would one PR work for all of them or would you prefer individual PRs for each item in the checklist(like this PR).
Edit:
I added code to the same branch as the last PR which is why there are 7 commits.