Skip to content

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

Merged
merged 11 commits into from
Aug 15, 2022
Merged

Conversation

srghma
Copy link
Member

@srghma srghma commented Feb 8, 2021

Description of the change

decodeURI and encodeURI -> add

#3

Checklist:

  • Added the change to the changelog's "Unreleased" section with a link to this PR and your username
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation in the README and/or documentation directory
  • Added a test for the contribution (if applicable)

Co-authored-by: Thomas Honeyman <admin@thomashoneyman.com>
@thomashoneyman
Copy link
Contributor

Other than those two comments I'm happy to merge this. Thanks!

@srghma
Copy link
Member Author

srghma commented Feb 8, 2021

@thomashoneyman done

@hjmtql
Copy link

hjmtql commented Aug 6, 2022

What is stopping this PR?
I tried to fix the latest commit d25d833 for purs v0.15.4.
It seemed to be fine.
hjmtql@5e7235e

I hope someone will review this again.

@JordanMartinez
Copy link
Contributor

Probably 2 things: the merge conflict above and just forgetting about the PR in the midst of many other things.

@JordanMartinez
Copy link
Contributor

@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).

@garyb
Copy link
Member

garyb commented Aug 13, 2022

I can take a look when it's fixed up if Thomas isn't around - these versions are for encoding whole URIs, whereas the *Component versions are for encoding just values in query strings, etc.

@thomashoneyman
Copy link
Contributor

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

@garyb
Copy link
Member

garyb commented Aug 15, 2022

Not sure what's needed to make spago happy here, but changes LGTM.

@JordanMartinez
Copy link
Contributor

@garyb The package set still refers to the prepare-0.15 package set, which no longer exists.

Copy link
Contributor

@JordanMartinez JordanMartinez left a 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.

@JordanMartinez
Copy link
Contributor

@garyb CI builds. Would you merge and release this?

@garyb
Copy link
Member

garyb commented Aug 15, 2022

Sure. Thanks for fixing the package set!

@garyb garyb merged commit 76f1053 into purescript-contrib:main Aug 15, 2022
@hjmtql
Copy link

hjmtql commented Aug 18, 2022

Thanks for quick fix!

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

Successfully merging this pull request may close these issues.

5 participants