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

implement isURLCodePoint #59

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

implement isURLCodePoint #59

wants to merge 9 commits into from

Conversation

cammytown
Copy link

Perhaps you were waiting since it's not a pretty thing to have to
implement. I'm doing it for my own purposes, so here's a version in what
I think is your style if you want it.

cammytown and others added 6 commits November 26, 2016 20:45
Perhaps you were waiting since it's not a pretty thing to have to
implement. I'm doing it for my own purposes, so here's a version in what
I think is your style if you want it.
changing single quotes to double quotes
Copy link
Member

@Sebmaster Sebmaster left a comment

Choose a reason for hiding this comment

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

Other changes look good.

function isURLCodePoint(c) {
return (
isASCIIAlphanumeric(c) ||
c === 0x21 ||
Copy link
Member

@Sebmaster Sebmaster Nov 27, 2016

Choose a reason for hiding this comment

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

Can you switch these over to use the notation used in the URL spec (that is, use the literal chars, pass them through our conversion p transform). Optionally change it to an array (not sure if that makes copying them off the spec easier)?

changed initial codePoint checks to check literal characters passed
through p() function; storing them in array
fixing indentation
@cammytown
Copy link
Author

implemented requested changes-- storing character codePoints into an array first even though this is not the strictest interpretation of of whatwg standard; not sure what preference would be.

also, my apologies if i made too many commits or named/described them improperly. i usually work alone so i'm really not experienced working on public-facing open-source projects.

thanks for your work on this.

@stevenvachon
Copy link
Contributor

No tests?

@Sebmaster
Copy link
Member

@stevenvachon There's no (public) API change in URL, it's basically just to match the spec more exactly like it's written.

@cammytown
Copy link
Author

@stevenvachon i'm not working in Javascript environment; just thought I'd write up the functionality since I was doing a similar implementation. changes were trivial

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

Successfully merging this pull request may close these issues.

3 participants