Add support for IRIs and RFC 3987 #50

sethmlarson · 2019-03-20T01:18:05Z

Closes #21, closes #31.

Adds a separate class IRIReference. What's left to be decided:

What is the best interface for using Validator and normalizers with IRIReference instances?
Should we include a default IDNA 2003 encoder or should we rely on users providing their own via the idna module?

sethmlarson · 2019-03-20T13:54:52Z

src/rfc3986/normalizers.py

@@ -162,6 +162,6 @@ def encode_component(uri_component, encoding):
                or (byte_ord < 128 and byte.decode() in misc.NON_PCT_ENCODED)):
            encoded_uri.extend(byte)
            continue
-        encoded_uri.extend('%{0:02x}'.format(byte_ord).encode())
+        encoded_uri.extend('%{0:02x}'.format(byte_ord).encode().upper())


So that percent-encoded bytes are normalized by default. :)

sigmavirus24

Regarding Validations, I think we want a URI as we can make assertions about that. In that sense I would guess someone instantiates an IRI, converts it to a URI and then performs validation.

sigmavirus24 · 2019-03-20T14:04:41Z

src/rfc3986/uri.py

-        ref.encoding = encoding
-        return ref
-
+class URIMixin:


A couple questions:

Should this be in its own "internal" (e.g., _mixin) namespace to avoid folks trying to use this somehow?

Do we want an old-style class on Python 2? If not should this be class URIMixin(object):?

I agree with the internal namespace, I'll implement that. And I can change this to a new-style class.

sigmavirus24 · 2019-03-20T14:06:37Z

src/rfc3986/iri.py

+            encoding,
+        )
+
+    def encode(self, idna_encoder=None):


On the one hand, I like the idea of users being able to provide their own encoder, but I wonder what the value of that is. There's really only one package I'm aware that does idna correctly (https://github.com/kjd/idna) which also handles IDNA 2008.

I admit, it's dreadfully slow to import. But do we really want to start with 2003 and work our way forwards by building our own?

I'd like to keep the idna module as an optional dependency because urllib3 doesn't directly depend on it, urllib3 only needs to be able to parse IRIs, it doesn't do the encoding itself.

Maybe we can try to import it and if it's not there and we need it to encode the host correctly we error out?

Something like:

try: import idna except ImportError: idna = None if idna_encoder is None and idna is None and host_needs_encoding: raise MissingDependencyError("Need idna installed in order to encode {}".format(host))

I could live with. It's not great but it could work. Also we could fallback on to IDNA 2003 on Py3 if we wanted to. I'd also like to see an extra added for that though named something like with_idna2008.

sigmavirus24 · 2019-03-20T15:18:42Z

src/rfc3986/iri.py

+            if self.port is not None:
+                authority += ":" + str(self.port)
+
+        return uri.URIReference(self.scheme,


Do we want to return this unconditionally?

What conditions would we be unable to encode an IRI into a URI?

If no encoder is provided? I think I've also seen idna.encode fail but I could be wrong

That is true, was wondering if you wanted it to fail if we fail to produce a valid URI but that's not necessary as the user can validate after we give the user a URI.

sigmavirus24 · 2019-03-24T15:10:06Z

So I think we're just waiting on the reoganization, right? I kind of want IRI support to be a "beta" feature to allow us to play with the API as we find necessary but 🤷‍♂️ I don't know how to do that appropriately

sethmlarson · 2019-03-24T15:20:27Z

Yeah, I can try to get to your review comments today. What are your thoughts on putting everything to do with IRIs into a private module until promotion (_iri, _beta?) or doing a pre-release with a beta flag? Either way urllib3 can still bundle and we can make changes to the IRI interface if we find any problems down the line.

sigmavirus24 · 2019-03-24T15:24:52Z

Eh, I don't care about hiding the module, just adding warning signs of sorts

sethmlarson · 2019-03-24T15:28:50Z

Would adding some warnings to users in documentation and the changelog be good enough here?

sigmavirus24 · 2019-03-25T11:39:10Z

Definitely

sigmavirus24 · 2019-04-12T11:44:50Z

Py2 is failing to encode unicode outside of the ascii range into an ascii string, perhaps we should use u"" in the raise InvalidAuthority(...) line.
Py3 seems to be failing on test coverage https://travis-ci.org/python-hyper/rfc3986/jobs/519049856#L1817
There are a bunch of Flake8 failures: https://travis-ci.org/python-hyper/rfc3986/jobs/519049858

sethmlarson · 2019-04-12T12:04:11Z

Yeah this commit is definitely not ready, wish there was a way to put the PR in a state that it wouldn't alert reviewers until you're actually ready. :)

I pushed so i could access these changes on a different machine.

sigmavirus24 · 2019-04-12T12:10:25Z

I think draft PRs do that but I don't think you from PR -> Draft PR

sethmlarson · 2019-04-12T12:17:56Z

I didn't know about draft PRs, thanks!

codecov-io · 2019-04-20T05:19:01Z

Codecov Report

Merging #50 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #50   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          11     13    +2     
  Lines         711    797   +86     
=====================================
+ Hits          711    797   +86

Impacted Files	Coverage Δ
src/rfc3986/normalizers.py	`100% <100%> (ø)`	⬆️
src/rfc3986/iri.py	`100% <100%> (ø)`
src/rfc3986/exceptions.py	`100% <100%> (ø)`	⬆️
src/rfc3986/abnf_regexp.py	`100% <100%> (ø)`	⬆️
src/rfc3986/__init__.py	`100% <100%> (ø)`	⬆️
src/rfc3986/uri.py	`100% <100%> (ø)`	⬆️
src/rfc3986/misc.py	`100% <100%> (ø)`	⬆️
src/rfc3986/api.py	`100% <100%> (ø)`	⬆️
src/rfc3986/_mixin.py	`100% <100%> (ø)`
... and 1 more

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 9a87fd6...f10931d. Read the comment docs.

sigmavirus24 · 2019-04-20T14:04:08Z

This looks good to me, @sethmlarson! Feel free to merge/release when you're ready

sethmlarson · 2019-04-20T14:37:56Z

Thanks @sigmavirus24! Did you want me to do the release (would need PyPI maintainer) or are you willing to? I'll update the release date to today in the docs before merge.

I really appreciate you doing these reviews despite being away from OSS, by the way.

sigmavirus24 · 2019-04-20T17:40:55Z

I'll add you to the package @sethmlarson :) And I really want to see urllib3 succeed so I'm happy to make tiny exceptions

sethmlarson · 2019-04-20T18:10:23Z

@sigmavirus24 Thanks much! I'll make a release today. :)

Add initial support for IRIs

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

0e83cce

sethmlarson requested a review from sigmavirus24 March 20, 2019 01:18

sethmlarson mentioned this pull request Mar 20, 2019

Use rfc3986.validator.Validator for parse_url urllib3/urllib3#1531

Merged

Undo accidental removal of escapes

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

d947ab6

sethmlarson commented Mar 20, 2019

View reviewed changes

sigmavirus24 reviewed Mar 20, 2019

View reviewed changes

sethmlarson mentioned this pull request Apr 8, 2019

Add support for TLS 1.3 to all HTTPSConnection implementations urllib3/urllib3#1496

Merged

Add mixin module and idna2008 extra

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

1bfd2e3

sethmlarson added 2 commits April 19, 2019 23:50

Add tests and documentation

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

5a01518

Fix documentation and lint issues

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

782f515

One more lint issue

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

7332f49

Update release date

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

f10931d

sethmlarson merged commit df21c4b into python-hyper:master Apr 20, 2019

sethmlarson deleted the iri branch April 20, 2019 14:48

Add support for IRIs and RFC 3987 #50

Add support for IRIs and RFC 3987 #50

Conversation

sethmlarson commented Mar 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigmavirus24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigmavirus24 commented Mar 24, 2019

Uh oh!

sethmlarson commented Mar 24, 2019

Uh oh!

sigmavirus24 commented Mar 24, 2019

Uh oh!

sethmlarson commented Mar 24, 2019

Uh oh!

sigmavirus24 commented Mar 25, 2019

Uh oh!

sigmavirus24 commented Apr 12, 2019

Uh oh!

sethmlarson commented Apr 12, 2019

Uh oh!

sigmavirus24 commented Apr 12, 2019

Uh oh!

sethmlarson commented Apr 12, 2019

Uh oh!

codecov-io commented Apr 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sigmavirus24 commented Apr 20, 2019

Uh oh!

sethmlarson commented Apr 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sigmavirus24 commented Apr 20, 2019

Uh oh!

sethmlarson commented Apr 20, 2019

Uh oh!

codecov-io commented Apr 20, 2019 •

edited

Loading

sethmlarson commented Apr 20, 2019 •

edited

Loading