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.

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

Jump to bottom

Warn if package index gets unexpected Content-Type #8083

Merged
merged 3 commits into from
May 23, 2020
Merged

Warn if package index gets unexpected Content-Type #8083

merged 3 commits into from
May 23, 2020

Conversation

deveshks
Copy link
Contributor

Fixes and closes #6754

@deveshks deveshks force-pushed the warn-on-unexpected-content-type branch from d56f79e to c66149b Compare April 19, 2020 09:41
@sbidoul
Copy link
Member

sbidoul commented Apr 19, 2020

I note all error reports in this area are debug level. Does it make sense to change only one of them? On the contrary, would it make sense to change all of them?

@deveshks
Copy link
Contributor Author

deveshks commented Apr 19, 2020 •
edited
Loading

I note all error reports in this area are debug level. Does it make sense to change only one of them? On the contrary, would it make sense to change all of them?

I think we can change https://github.com/pypa/pip/blob/master/src/pip/_internal/index/collector.py#L437 and https://github.com/pypa/pip/blob/master/src/pip/_internal/index/collector.py#L453 , both to logger.warning, since they both look like user errors in providing arguments to pip command line, and can be potentially helpful to show it to them

Perhaps: https://github.com/pypa/pip/blob/master/src/pip/_internal/index/collector.py#L411 as well, but this lies outside _get_html_page

@deveshks deveshks force-pushed the warn-on-unexpected-content-type branch 2 times, most recently from 980359e to 103a4d6 Compare April 24, 2020 11:08
@deveshks
Copy link
Contributor Author

As per #8083 (comment) , can I extend the scope of this issue, as well as the associated PR to include the remaining two logger.debug messages which actually contain an error to logger.warning, or should I do that perhaps as part of another PR (I am more in favor of handling this as part of another PR)

pfmoore
pfmoore approved these changes Apr 24, 2020
View reviewed changes
Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

One minor wording nit

src/pip/_internal/index/collector.py Outdated Show resolved Hide resolved
@pfmoore
Copy link
Member

pfmoore commented Apr 24, 2020

I'm happy to have changing the other messages' level be in a separate PR. I'm not entirely sure that the other messages even should be changed to warnings - IMO that should be discussed and decided on a case by case basis, so a separate PR seems correct.

@deveshks deveshks mentioned this pull request Apr 24, 2020
Closed
@deveshks
Copy link
Contributor Author

I'm happy to have changing the other messages' level be in a separate PR. I'm not entirely sure that the other messages even should be changed to warnings - IMO that should be discussed and decided on a case by case basis, so a separate PR seems correct.

I have gone ahead and create an issue to discuss this #8128 , and get some agreement before creating a PR for the same

@deveshks
Copy link
Contributor Author

If there are no more changes necessary to be done in this PR, may I get this merged?

@deveshks deveshks force-pushed the warn-on-unexpected-content-type branch from 91c01e3 to 64c78b1 Compare May 21, 2020 14:07
@pradyunsg pradyunsg added C: finder PackageFinder and index related code type: enhancement Improvements to functionality labels May 23, 2020
@pradyunsg pradyunsg merged commit d5265e2 into pypa:master May 23, 2020
@deveshks deveshks deleted the warn-on-unexpected-content-type branch May 23, 2020 11:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pfmoore pfmoore pfmoore approved these changes

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
C: finder PackageFinder and index related code type: enhancement Improvements to functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pip silently ignores index pages with unexpected content type
4 participants
@deveshks @sbidoul @pfmoore @pradyunsg