Skip to content

Standardise link repr in logs #11083

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −11
Open

Standardise link repr in logs #11083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/7390.trivial.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Standardize ``Link`` representation in logs.
5 changes: 4 additions & 1 deletion src/pip/_internal/index/collector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,10 @@ def _handle_get_page_fail(
) -> None:
if meth is None:
meth = logger.debug
meth("Could not fetch URL %s: %s - skipping", link, reason)
url = str(link)
if logger.getEffectiveLevel() > logging.DEBUG:
url = link.show_url
meth("Could not fetch URL %s: %s - skipping", url, reason)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think warnings/errors should present partial URLs, since its possible that the user has multiple indexes and needs to know which one the error is related to, without adding additional verbosity.



def _make_html_page(response: Response, cache_link_parsing: bool = True) -> HTMLPage:
Expand Down
9 changes: 2 additions & 7 deletions src/pip/_internal/network/download.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

from pip._internal.cli.progress_bars import get_download_progress_renderer
from pip._internal.exceptions import NetworkConnectionError
from pip._internal.models.index import PyPI
from pip._internal.models.link import Link
from pip._internal.network.cache import is_from_cache
from pip._internal.network.session import PipSession
Expand All @@ -34,11 +33,7 @@ def _prepare_download(
) -> Iterable[bytes]:
total_length = _get_http_response_size(resp)

if link.netloc == PyPI.file_storage_domain:
url = link.show_url
else:
url = link.url_without_fragment

url = link.show_url
Copy link
Member

Choose a reason for hiding this comment

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

This is the only part of the change that I'm somewhat fine with, but I wonder if show_url should also mention the domain. That's something we can address in a follow up though.

logged_url = redact_auth_from_url(url)

if total_length:
Expand Down Expand Up @@ -170,7 +165,7 @@ def __call__(
logger.critical(
"HTTP error %s while getting %s",
e.response.status_code,
link,
link.show_url,
Copy link
Member

Choose a reason for hiding this comment

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

This should not be show_url but the entire URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So its still inconsistent link repr in logs - as described in the issue

)
raise

Expand Down
6 changes: 3 additions & 3 deletions tests/unit/test_network_download.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,19 @@
"http://example.com/foo.tgz",
{},
False,
"Downloading http://example.com/foo.tgz",
"Downloading foo.tgz",
),
(
"http://example.com/foo.tgz",
{"content-length": "2"},
False,
"Downloading http://example.com/foo.tgz (2 bytes)",
"Downloading foo.tgz (2 bytes)",
),
(
"http://example.com/foo.tgz",
{"content-length": "2"},
True,
"Using cached http://example.com/foo.tgz (2 bytes)",
"Using cached foo.tgz (2 bytes)",
),
("https://files.pythonhosted.org/foo.tgz", {}, False, "Downloading foo.tgz"),
(
Expand Down