8000 Add `requireSchemeOrWww` option by osdiab · Pull Request #51 · sindresorhus/get-urls · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add requireSchemeOrWww option #51

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
Merged
merged 11 commits into from
Oct 18, 2019
Merged

Add requireSchemeOrWww option #51

merged 11 commits into from
Oct 18, 2019

Conversation

osdiab
Copy link
Contributor
@osdiab osdiab commented Oct 16, 2019 •
edited by sindresorhus
Loading

Fixes #48
Fixes #24

Not sure how this ought to work with the extractFromQueryString param since I never use that - should it just forward the flag there?

@sindresorhus
Copy link
Owner

The option name makes sense for the regex, but not here, as including www and protocol-less URLs is actually looser. I think it should be called requireScheme and default to true.

@sindresorhus
Copy link
Owner

Not sure how this ought to work with the extractFromQueryString param since I never use that - should it just forward the flag there?

I don't think it should apply there as it will be too ambiguous.

@osdiab
Copy link
Contributor Author
osdiab commented Oct 16, 2019

@sindresorhus edited with comments

@osdiab osdiab changed the title Add strict url parsing flag Add requireScheme flag Oct 16, 2019
@sindresorhus
Copy link
Owner

Hmm, sorry, I missed that it also affects www, so the option should be requireSchemeOrWww to be explicit. Also use the scheme word instead of protocol.

@sindresorhus
Copy link
Owner

Maybe also add a note that when it’s off it will match things like unicorn.education.

@osdiab osdiab changed the title Add requireScheme flag Add requireSchemeOrWww flag Oct 16, 2019
@osdiab
Copy link
Contributor Author
osdiab commented Oct 16, 2019

back to you @sindresorhus

@nestarz
Copy link
nestarz commented Oct 17, 2019

Can we give the user the hand to the list of the TLD ? For example, .onions are not captured using this.

@osdiab
Copy link
Contributor Author
osdiab commented Oct 17, 2019

I actually added an issue to url-regex earlier for that exact request, although for a different reason: kevva/url-regex#65

Would be helpful to advocate your reason for wanting this there!

With that in mind maybe it is better to just do #24 instead for maximum flexibility with less API surface, what do you think @sindresorhus ?

@osdiab osdiab 8000 mentioned this pull request Oct 18, 2019
Open
@osdiab
Copy link
Contributor Author
osdiab commented Oct 18, 2019 •
edited
Loading

@nestarz made a PR for that feature, which if passed in some form maybe can end up here too. kevva/url-regex#66

@sindresorhus sindresorhus changed the title Add requireSchemeOrWww flag Add requireSchemeOrWww option Oct 18, 2019
@sindresorhus
Copy link
Owner

With that in mind maybe it is better to just do #24 instead for maximum flexibility with less API surface, what do you think @sindresorhus ?

No, I want to explicitly only expose options that make sense.

@sindresorhus sindresorhus merged commit 5daceb8 into sindresorhus:master Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add option to get URLs without a protocol Support url-regex options
3 participants
@osdiab @sindresorhus @nestarz
0