-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Drop the doctype check #10906
Drop the doctype check #10906
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't clear from @pradyunsg 's comment whether he meant get rid of the doctype validation for index pages (which, technically, are covered by a PEP) in addition to --find-links
parsing. This does both, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but, obviously I'm not a pip maintainer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably also have an explicit test that --find-links
pointing to a page without a doctype declaration works. Such a test would ensure that in future, even if we tighten up the validation for indexes, we don't accidentally require the same strictness for --find-links
.
tests/unit/test_collector.py
Outdated
@@ -586,7 +586,7 @@ def test_parse_links_presents_warning_on_html4_doctype( | |||
assert "pkg1-1.0" in parsed_links[0].url | |||
assert "pkg1-2.0" in parsed_links[1].url | |||
|
|||
assert len(caplog.records) == 1 | |||
assert len(caplog.records) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd strongly prefer to drop these tests instead of promising that we won't warn on invalid/missing doctypes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to do that. But I'd still like to see a test that ensures that any validation we might put on indexes doesn't "leak" into how we handle --find-links
(because --find-links
is explicitly intended to support anything that dumps out an unstructured "bunch of links to files", such as a webserver auto-generated directory page). But I won't block somebody else merging this PR without that test, if people feel I'm being too pedantic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ill add a test for --find-links
.
Tests are failed because of this: pypa/flit#498 |
@q0w Do you know when you might be able to update this PR? |
@pradyunsg today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll wait for @pfmoore to take a look, but LGTM!
This comment was marked as resolved.
This comment was marked as resolved.
I can use the squash button! Thanks @q0w! ^.^ |
Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>
Closes #10903