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

Can twisted.python.url.URL be switched over to hyperlink.DecodedURL by default? #60

Closed
mahmoud opened this issue Jan 11, 2018 · 8 comments
Labels

Comments

@mahmoud
Copy link
Member

mahmoud commented Jan 11, 2018

Just wanted to open a discussion. Previously, Twisted's URL didn't mind much if reserved characters were added in values (see #6 and #8). Hyperlink's URL changed that, with all that entailed (see #44). Now, DecodedURL would allow a return to all-characters allowed, with the necessary escaping happening automatically.

Would it make sense for DecodedURL to become Twisted's primary URL? It wasn't designed to, but it's pretty close, with at least one exception (userinfo is a tuple instead of a :-separated string).

@glyph @markrwilliams @wsanchez, thoughts?

@wsanchez
Copy link
Contributor

The reason I'm not a fan of the heuristic approach that hyperlink takes with parsing URLs is that I'm not writing code for a web browser, or something else asking for user input or rando input; inside of a web server, you should be dealing with something more strict and canonical.

So that's why I like DecodedURL for my purposes. As to Twisted… I think we should deprecate twisted.python.URL and make people decide which URL semantics they want, because Twisted clients include both web servers (eg. my use case) and clients that do take user input.

We've already changed the behavior of twisted.python.URL, and I'm not sure that changing it back is less harmful than leaving it alone… my inclination is to use DecodedURL there, but that's probably biased by my use case.

@glyph
Copy link
Collaborator

glyph commented Jan 23, 2018

As phrased, there's a simple answer: "no". The compatibility earthquakes that have already occurred around this should not be repeated :).

@glyph
Copy link
Collaborator

glyph commented Jan 23, 2018

As for Twisted, we should add a method to Request and that method should absolutely return a DecodedURL.

@glyph
Copy link
Collaborator

glyph commented Jan 23, 2018

(twisted.python.url should itself simply be deprecated, and people should be encouraged to import the type from where it actually lives.)

@glyph
Copy link
Collaborator

glyph commented Jan 23, 2018

My suggestion would be to close this issue since even if there's a conclusion around this, it's not work that needs to be done on DecodedURL. This discussion might be appropriate to the Twisted mailing list, though.

@mahmoud
Copy link
Member Author

mahmoud commented Jan 23, 2018

@glyph Happy to close the issue once discussion has concluded, not sure if anyone else has a reply. This is sort of meant to gather a bit of feedback from Twisted developers present as to whether something can be done to make DecodedURL more legacy-t.p.url.URL-like.

For instance, the compatibility earthquake happened, but it wasn't that long ago. A lot of the dependencies who might be affected probably haven't even seen the first earthquake. They might see a much less severe earthquake with this autoescaping approach, which is safer and requires less work on their part.

If we're willing to forgo absolute correctness around userinfo (which would require a tweak to DecodedURL as currently written), the API would be almost identical.

@glyph
Copy link
Collaborator

glyph commented Oct 18, 2018

(It looks like nobody else had a reply? Do we need to wait another 10 months for the discussion to conclude? :))

@wsanchez
Copy link
Contributor

(It looks like nobody else had a reply? Do we need to wait another 10 months for the discussion to conclude? :))

Longer, actually. :-)

Closing.

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

No branches or pull requests

3 participants