-
Notifications
You must be signed in to change notification settings - Fork 8
decodeURI and encodeURI -> add #4
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
Conversation
Co-authored-by: Thomas Honeyman <admin@thomashoneyman.com>
Other than those two comments I'm happy to merge this. Thanks! |
@thomashoneyman done |
What is stopping this PR? I hope someone will review this again. |
Probably 2 things: the merge conflict above and just forgetting about the PR in the midst of many other things. |
@srghma Could you fix the linting errors breaking CI? @thomashoneyman Since you originally reviewed this, does this look good to you? I don't quite understand what this change does or why it's needed, but it seems like you do. (Or perhaps one of the others here can clarify for me). |
I can take a look when it's fixed up if Thomas isn't around - these versions are for encoding whole URIs, whereas the |
I think it’s sensible to have these versions. To see how they differ from say encodeURIComponent, I’d recommend MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURI |
Not sure what's needed to make spago happy here, but changes LGTM. |
@garyb The package set still refers to the |
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.
Just to get that 2nd approval. I'm just trusting Gary on this one.
@garyb CI builds. Would you merge and release this? |
Sure. Thanks for fixing the package set! |
Thanks for quick fix! |
Description of the change
decodeURI and encodeURI -> add
#3
Checklist: