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.

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

Fix URL parsing with Python 3.8.1 and 2.7.15 #409

Closed
wants to merge 1 commit into from

Conversation

onovy
Copy link

@onovy onovy commented Dec 28, 2019

What do these changes do?

URL parsing has been changed in newer Python releases, see: https://bugs.python.org/issue27657

Are there changes in behavior for the user?

No, just test fix.

Related issue number

None.

Checklist

  • I think the code is well written
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

@codecov
Copy link

codecov bot commented Dec 28, 2019

Codecov Report

Merging #409 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #409   +/-   ##
=======================================
  Coverage   99.24%   99.24%           
=======================================
  Files           2        2           
  Lines         664      664           
  Branches      152      152           
=======================================
  Hits          659      659           
  Misses          5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be1251a...add2ab1. Read the comment docs.

@bigon
Copy link

bigon commented Mar 8, 2020

Are you sure this is OK?

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=952211

@bigon
Copy link

bigon commented Mar 8, 2020

I deleted my last comment as it was incorrect

According to https://bugs.python.org/issue27657, they have reverted the fix in 3.8.2 and 3.7.7 as it was causing unexpected behavior changes in a minor release.

So the patch should look more like the following if I understand well:

--- a/tests/test_url_parsing.py
+++ b/tests/test_url_parsing.py
@@ -40,9 +40,15 @@ class TestScheme:

     def test_no_scheme1(self):
         u = URL("google.com:80")
-        assert u.scheme == ""
-        assert u.host is None
-        assert u.path == "google.com:80"
+        # See: https://bugs.python.org/issue27657
+        if sys.version_info[:3] == (3, 7, 6) or sys.version_info[:3] == (3, 8, 1) or sys.version_info >= (3, 9, 0):
+            assert u.scheme == "google.com"
+            assert u.host is None
+            assert u.path == "80"
+        else:
+            assert u.scheme == ""
+            assert u.host is None
+            assert u.path == "google.com:80"
         assert u.query_string == ""
         assert u.fragment == ""

@onovy
Copy link
Author

onovy commented Mar 9, 2020

Are you sure this is OK?

yes, I was sure it was right. Change in Python was reverted after this PR.

I checked your patch and looks correct. Updated this PR to reflect that.

Thanks.

@bigon
Copy link

bigon commented Mar 9, 2020

Thanks

@asvetlov
Copy link
Member

Merged manually by 3d5bf8f
Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants