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

Stop percent-encoding of numbers. #4

Merged
merged 1 commit into from Aug 8, 2016

Conversation

Projects
None yet
5 participants
@DanTup
Member

DanTup commented Jul 24, 2016

RFC 3986 states that numbers are unreserved characters:

Characters that are allowed in a URI but do not have a reserved purpose are called unreserved. These include uppercase and lowercase letters, decimal digits, hyphen, period, underscore, and tilde.

unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"

[...]

For consistency, percent-encoded octets in the ranges of ALPHA (%41-%5A
and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E), underscore
(%5F), or tilde (%7E) should not be created by URI producers

There were no tests or code specifically handling numbers so I believe this is just an oversight. This change adds numbers to the test of reserved characters as well as a couple of the additional tests that had alpha characters in.

This fixes #3.

I've signed the CLA but I'm not sure how to submit this for review (the contributing file doesn't explain much); is that done here in the PR or should do I need to do something to add this to codereview.chromium.org system?

Stop percent-encoding of numbers.
RFC 3986 states that numbers are unreserved characters:

Characters that are allowed in a URI but do not have a reserved purpose
are called unreserved. These include uppercase and lowercase letters,
decimal digits, hyphen, period, underscore, and tilde.

unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"
[...]

For consistency, percent-encoded octets in the ranges of ALPHA (%41-%5A
and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E), underscore
(%5F), or tilde (%7E) should not be created by URI producers
@devoncarew

This comment has been minimized.

Member

devoncarew commented Aug 5, 2016

@lrhn

This comment has been minimized.

Member

lrhn commented Aug 8, 2016

Sorry for the delay, I'm back from vacation now. I have also made a similar patch CL at https://codereview.chromium.org/2225763002/ (although your CL seems to have better testing!)

@DanTup

This comment has been minimized.

Member

DanTup commented Aug 8, 2016

@lrhn Cool; you're free to just trash this PR if you've already pushed code, as long as it's fixed I'm happy :-)

@googlebot googlebot added the cla: yes label Aug 8, 2016

@nex3

This comment has been minimized.

Member

nex3 commented Aug 8, 2016

I'm going to merge in this one, since it has tests and I think the test is a little clearer.

@nex3 nex3 merged commit 44b5c66 into dart-lang:master Aug 8, 2016

1 check passed

cla/google All necessary CLAs are signed
@DanTup

This comment has been minimized.

Member

DanTup commented Aug 9, 2016

Great; thanks for pushing this out in a release already! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment