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.

Sign up for GitHub

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

Jump to bottom

fix(buildUrl): path encoding #35

Merged
merged 4 commits into from
Apr 12, 2021
Merged

fix(buildUrl): path encoding #35

merged 4 commits into from
Apr 12, 2021

Conversation

luqven
Copy link
Contributor

@luqven luqven commented Apr 5, 2021 •
edited
Loading

Description

This PR ensures that + get encoded properly by the buildUrl method and adds relevant tests.

@luqven luqven self-assigned this Apr 5, 2021
@commit-lint
Copy link

commit-lint bot commented Apr 5, 2021 •
edited
Loading

Tests

  • reservedChars: ensure +:?# get encoded (217077d)
  • unicode: test unencoded unicode gets encoded (542fc00)

Chore

  • add ixEncodeUriCharSet & ixEncodeUri (197c7d7)

Bug Fixes

  • sanitizePath: encode plus with ixEncodeURI (0a28b20)

Contributors

luqven

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

sherwinski
sherwinski approved these changes Apr 6, 2021
View reviewed changes
Copy link
Contributor

@sherwinski sherwinski left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@luqven luqven marked this pull request as ready for review April 8, 2021 18:46
@luqven luqven requested a review from a team as a code owner April 8, 2021 18:46
ericdeansanchez
ericdeansanchez approved these changes Apr 8, 2021
View reviewed changes
Copy link

@ericdeansanchez ericdeansanchez left a comment

Choose a reason for hiding this comment

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

👍 I left some comments about us trying to do the percent encoding without having to use a replacement function to do so after adding the initial encoding, but that's a "nice to have" at this point. If we can do it (and it's easy) great, if not––no worries.

@luqven luqven force-pushed the luis/pathEncoding branch from eb6862b to 40aaf22 Compare April 9, 2021 02:45
@luqven luqven force-pushed the luis/pathEncoding branch from 40aaf22 to b6b9523 Compare April 12, 2021 13:56
Base automatically changed from luis/vscode to main April 12, 2021 20:32
@luqven luqven force-pushed the luis/pathEncoding branch from b6b9523 to 0a28b20 Compare April 12, 2021 20:33
@luqven luqven merged commit 71371ee into main Apr 12, 2021
@luqven luqven deleted the luis/pathEncoding branch April 12, 2021 20:38
@luqven luqven mentioned this pull request Apr 13, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherwinski sherwinski sherwinski approved these changes

@ericdeansanchez ericdeansanchez Awaiting requested review from ericdeansanchez

Assignees

@luqven luqven

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@luqven @sherwinski @ericdeansanchez