Adding parse and DecodedURL docs - #126
Conversation
…ault argument to DecodedURL() to ease programmatic construction. Fixes #125
| construct a :class:`DecodedURL`, you can use this pattern: | ||
|
|
||
| >>> DecodedURL().replace(host='pypi.org', path=('projects', 'hyperlink').to_text() | ||
| "http://pypi.org/projects/hyperlink" |
There was a problem hiding this comment.
Any opinion on adding a DecodedURL.build(...) or is this a fine pattern?
|
@twm if you've got a sec, please take a look and let me know what you think. thanks! |
twm
left a comment
twm
left a comment
There was a problem hiding this comment.
Thank you for jumping on this so quickly! I had a few thoughts. I'm also happy to help out on either the implementation or documentation sides — just let me know.
|
I should say more clearly: the documentation changes here are exactly what I was looking for. Thank you! |
Codecov Report
@@ Coverage Diff @@
## master #126 +/- ##
==========================================
+ Coverage 98.55% 98.81% +0.25%
==========================================
Files 10 10
Lines 1595 2020 +425
Branches 187 288 +101
==========================================
+ Hits 1572 1996 +424
Misses 12 12
- Partials 11 12 +1
Continue to review full report at Codecov.
|
|
OK I forked off the AbstractURL discussion to #127. With that taken care of, I think these docs answer far more questions than they raise, so if there are no critical blockers, I'd like to get them deployed sooner rather than later. Any approvers? |
| import hyperlink | ||
|
|
||
| url = URL.from_text(u'http://github.com/python-hyper/hyperlink?utm_source=readthedocs') | ||
| url = hyperlink.parse(u'http://github.com/python-hyper/hyperlink?utm_source=readthedocs') |
There was a problem hiding this comment.
FWIW I'm not a fan of parse, which can return two types, and would recommend that one use URL.from_text or DecodedURL.from_text directly instead, so this change seems like a minus to me, but I might be alone there.
There was a problem hiding this comment.
Yeah I think parse() was added to shift recommended practice toward DecodedURLs, because they're safer. parse() returns a DecodedURL by default. I think it also looks pretty nice.
| """ | ||
|
|
||
| def __init__(self, url, lazy=False): | ||
| def __init__(self, url=_EMPTY_URL, lazy=False): |
There was a problem hiding this comment.
why the default? Perhaps merits discussion outside of doc changes.
There was a problem hiding this comment.
Sure, a couple reasons:
- It's more parallel with
URL(), which can be instantiated without arguments to get an empty URL. - This way to programmatically construct a
DecodedURL, you don't need to also importURL. You can doDecodedURL().replace(...).
There was a problem hiding this comment.
Ah, and the reason for including it in these changes was because I really wanted to avoid people programmatically constructing the newly-exposed DecodedURL by doing
from hyperlink import URL, DecodedURL
DecodedURL(URL(...))
Without realizing that URL's initializer arguments aren't as rigorously decoded/encoded as the rest of DecodedURL's API.
I proposed adding a .build() or .from_parts() if we want to shift to a better pattern. For now it's either that or parse('').replace(...). I figured a default arg at least lets you explicitly use the type without an extra parse()/from_text() step.
There was a problem hiding this comment.
from_parts() sounds like a good addition. I have written DecodedURL(URL()) — it's a bit awkward. Perhaps I am worrying too much (this is Python, everything is slow...) but I tend to look for APIs that avoid temporaries.
There was a problem hiding this comment.
OK makes sense, and yes to from_parts.
wsanchez
left a comment
wsanchez
left a comment
There was a problem hiding this comment.
I think this is an improvement, though I'm unsure why the default to DecodedURL needs to be added at all, and especially as part of this PR.
I don't need to hold things up if you disagree, though.
|
codecov is having some system issues right now, I can't get codecov to come back with the coverage. The report here is saying it's gone up and is around 98.56%. Going to admin merge on that basis. |
4 participants
#125 correctly points out,
hyperlink.parse()andhyperlink.DecodedURL()aren't documented, so I put together something quick.I think we maybe talked about having URL become DecodedURL at some point, but I'm not sure it's worth waiting for that.
I think this PR basically brings the docs in line with recommended practices for using hyperlink.
Questions reviewers in the comments.