Skip to content

bpo-29976: urllib.parse clarify '' in scheme values.#984

Merged
orsenthil merged 4 commits into
python:masterfrom
orsenthil:urlparse_default
May 18, 2017
Merged

bpo-29976: urllib.parse clarify '' in scheme values.#984
orsenthil merged 4 commits into
python:masterfrom
orsenthil:urlparse_default

Conversation

@orsenthil

Copy link
Copy Markdown
Member

Issue for discussion: http://bugs.python.org/issue29976

@mention-bot

Copy link
Copy Markdown

@orsenthil, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @ncoghlan and @Yhg1s to be potential reviewers.

@terryjreedy

Copy link
Copy Markdown
Member

Add 'bpo-29976: ' to the beginning of the title to invoke auto-linking of PR and status back to the issue.

@orsenthil orsenthil changed the title urllib.parse clarify '' in scheme values. bpo-29976: urllib.parse clarify '' in scheme values. Apr 7, 2017
@orsenthil

Copy link
Copy Markdown
Member Author

@terryjreedy - thanks for the note, I missed that one!

Comment thread Lib/urllib/parse.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'de facto', without '-'. See for instance https://www.merriam-webster.com/dictionary/defacto

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Changed.

Comment thread Lib/urllib/parse.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better, but without having used argparse yet, I think something like "Use ' ' as the scheme value to get the default scheme in urlsplit and urlparse." might be even better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I settled for this. # We use '' as the scheme value for default scheme in urlsplit and urlparse.

Comment thread Lib/urllib/parse.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uses_relative only seems to affect “urljoin”, not “urlsplit” nor “urlparse”. The empty string means that you can join one relative URL onto another one.

@orsenthil orsenthil Apr 9, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment applies to all three lists, uses_relative, uses_netloc and uses_params.
All of them have '' (empty string) listed as supported scheme, because that is the default
def urlsplit(url, scheme='', allow_fragments=True):
and
def urlparse(url, scheme='', allow_fragments=True):

And the current comment is:
# A classification of schemes ('' means apply by default)

  • I think, the existing comment is confusing, and thus, this change is to make it a bit clear.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I misunderstood the comment. I thought you were listing the functions that the lists affected. What about something like

The empty string classifies relative URLs with no scheme specified, being the default returned by “urlsplit” and “urlparse”

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. I will write it as "default value".

The empty string classifies relative URLs with no scheme specified, being the default value returned by “urlsplit” and “urlparse”

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified the comment as per our discussion here.

@berkerpeksag

Copy link
Copy Markdown
Member

IMO the changes to __doc__ should go into another PR. Right now, diff is a bit noisy and it will clutter the purpose of git blame.

@orsenthil

Copy link
Copy Markdown
Member Author

Removed the unrelated docstring changes and I guess, this is good to go.

@orsenthil orsenthil merged commit 906f533 into python:master May 18, 2017
orsenthil added a commit that referenced this pull request May 21, 2017
orsenthil added a commit that referenced this pull request May 21, 2017
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

Successfully merging this pull request may close these issues.

6 participants