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

Improve URL encoding #7997

Merged
merged 2 commits into from Jul 29, 2019

Conversation

@straight-shoota
Copy link
Member

commented Jul 26, 2019

  • Adds methods URI.encode and URI.decode for URL encoding without
    escaping reserved characters. These are to be used when encoding URLs.
  • URI.escape and URI.unescape are renamed to URI.encode_www_form and
    URI.decode_www_form. These are to be used for x-www-form-urlencoded serialization.

The specific implementations are explained in the documentation.
Method names have been chosen according to the names of other APIs, especially Elixir's, per #3515 (comment) and #3515 (comment)

All these methods have been moved to a separate file in src/uri/encoding.cr to clean up the main file. They're an isolated domain just live in URI's class namespace.

There were two variants of escape/unescape receiving a block, which allow for custom modifications of the encoding rules. I'm not sure whether we need to expose them at all in the public API. Use cases should be pretty rare.
So maybe, we should nodoc them. I only kept the variant writing to IO. Such low-level methods to not need to provide a string variant. This at least reduces the impact on the public API a bit.

Closes #3515

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/uri-encode branch 2 times, most recently from c3ef91e to e9e1e13 Jul 26, 2019

Improve URL encoding
* Adds methods URI.encode and URI.decode for URL encoding without
  escaping reserved characters.
* URI.escape and URI.unescape are renamed to URI.encode_www_form and
URI.decode_www_form.

All these methods have been moved to a separate file in
`src/uri/encoding.cr` to clean up the main file. They're an isolated
domain just live in URI's class namespace.

Method names have been chosen according to the names of other APIs,
especially Elixir's (see #3515 (comment)).

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/uri-encode branch from e9e1e13 to ee15512 Jul 26, 2019

Show resolved Hide resolved src/uri.cr Outdated
@asterite
Copy link
Member

left a comment

This is beautiful, thank you!

Show resolved Hide resolved spec/std/uri_spec.cr Outdated
Show resolved Hide resolved src/http/cookie.cr Outdated
Show resolved Hide resolved src/uri.cr Outdated
@RX14

RX14 approved these changes Jul 29, 2019

@RX14 RX14 added this to the 0.30.0 milestone Jul 29, 2019

@RX14 RX14 merged commit d5860fc into crystal-lang:master Jul 29, 2019

4 of 5 checks passed

ci/circleci: test_linux32 Your tests failed on CircleCI
Details
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

@RX14 This branch could've used a rebase/squash before merging. Now we have this fixup in master: 2c416ba

@straight-shoota straight-shoota deleted the straight-shoota:jm/feature/uri-encode branch Jul 29, 2019

@RX14

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Whoops, I always forget that github remembers the "last" merge mode used.

@asterite

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

I personally don't think the commit history is that important.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

It's by far not critical, but keeping the history clean is somewhat important, so that the changes are comprehensible. If we had lot's of isolated fixup commits, it could be a nightmare to figure out how some change came to be.

@bcardiff bcardiff referenced this pull request Jul 31, 2019

Closed

Regression with cookies #8020

@bcardiff

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

I think I've found an issue with this PR. Reported as #8020

straight-shoota added a commit to straight-shoota/crystal that referenced this pull request Aug 2, 2019

@straight-shoota straight-shoota referenced this pull request Aug 2, 2019

Merged

Remove TODO #8033

RX14 added a commit that referenced this pull request Aug 2, 2019

@drujensen drujensen referenced this pull request Aug 7, 2019

Merged

Crystal v0.30.0 changes #1126

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.