Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding parse and DecodedURL docs #126
Adding parse and DecodedURL docs #126
Changes from 1 commit
a06eedb
1f57a36
4afeb55
6682bd9
b086352
182f834
2bae6f6
ca40c74
2d21b70
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I'm not a fan of
parse
, which can return two types, and would recommend that one useURL.from_text
orDecodedURL.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think
parse()
was added to shift recommended practice toward DecodedURLs, because they're safer.parse()
returns aDecodedURL
by default. I think it also looks pretty nice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any opinion on adding a
DecodedURL.build(...)
or is this a fine pattern?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the default? Perhaps merits discussion outside of doc changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, a couple reasons:
URL()
, which can be instantiated without arguments to get an empty URL.DecodedURL
, you don't need to also importURL
. You can doDecodedURL().replace(...)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 orparse('').replace(...)
. I figured a default arg at least lets you explicitly use the type without an extraparse()
/from_text()
step.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from_parts()
sounds like a good addition. I have writtenDecodedURL(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK makes sense, and yes to
from_parts
.