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

Add AbstractURL type #127

Open
mahmoud opened this issue Apr 14, 2020 · 2 comments
Open

Add AbstractURL type #127

mahmoud opened this issue Apr 14, 2020 · 2 comments

Comments

@mahmoud
Copy link
Member

mahmoud commented Apr 14, 2020

Per @wsanchez and @twm in #126, consensus seems to be that hyperlink would benefit from an interface/ABC which could be for both DecodedURL and URL/EncodedURL.

AbstractURL is the current name frontrunner, as URL is already taken for legacy reasons.

To quote @twm:

These methods seem to be equivalent between URL and DecodedURL in the sense that they do semantically identical things:

to_text()
to_uri()
to_iri()
normalize()
absolute
scheme
port
rooted

__bytes__ should probably be on this list (per #55). DecodedURL appears to lack it.

I'd also suggest that URL's get_decoded_url() method and DecodedURL's encoded_url property should be part of any common interface. This would be useful in treq to avoid a chain of isinstance() checks.

click() is also similar between the two, though only the DecodedURL version accepts a DecodedURL.

Further discussion to follow, no doubt.

@glyph
Copy link
Collaborator

glyph commented Apr 15, 2020

I think this is tempting but probably wrong. Pretty much nothing should be treating a decoded URL as encoded or vice-versa. What kind of code wants a shared type?

@twm
Copy link
Collaborator

twm commented Apr 16, 2020

In twisted/treq#279 I was looking to accept both. Almost all treq needs is the ability to ultimately convert to bytes (which you can do with the methods I listed as "equivalent" above).

There is a code path there that conditionally wants DecodedURL, though — merging the params argument (see twisted/treq#283, reviews appreciated!). So for this purpose it would be useful to do something like:

if params:
    decoded_url = parsed_url.get_decoded_url()
    parsed_url = decoded_url.replace(
        query=decoded_url.query + tuple(_coerced_query_params(params))
    ).encoded_url

So that is part of the motivation for my previous comment, particularly that get_decoded_url() and encoded_url should be present on both. Is that compelling? Eh, I'm not so sure. It's not much code in treq either way.

The other part of my motivation is that I wanted to make sure that URL.get() and DecodedURL.get() aren't part of any "shared" part. They are parallel, but distinct, so appearing in a base type would confuse matters. It is confusing enough that they have the same name.

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

No branches or pull requests

3 participants