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

allow domain names in urls not starting with www #50

Merged
merged 3 commits into from
Nov 20, 2024
Merged

Conversation

eoverly
Copy link
Collaborator

@eoverly eoverly commented Nov 5, 2024

One line description of your change (less than 72 characters)

Problem

Explain the context and why you're making that change. What is the problem
you're trying to solve? In some cases there is not a problem and this can be
thought of being the motivation for your change.

Solution

Describe the modifications you've done.

Result

What will change as a result of your pull request? Note that sometimes this
section is unnecessary because it is self-explanatory based on the solution.

Some important notes regarding the summary line:

  • Describe what was done; not the result
  • Use the active voice
  • Use the present tense
  • Capitalize properly
  • Do not end in a period — this is a title/subject
  • Prefix the subject with its scope

Test Plan

(Write your test plan here. If you changed any code, please provide us with
clear instructions on how you verified your changes work.)

/(https?:\/\/(?:www\.|(?!www))[a-zA-Z0-9][a-zA-Z0-9-]+[a-zA-Z0-9]\.[^\s]{2,}|www\.[a-zA-Z0-9][a-zA-Z0-9-]+[a-zA-Z0-9]\.[^\s]{2,}|https?:\/\/(?:www\.|(?!www))[a-zA-Z0-9]+\.[^\s]{2,}|www\.[a-zA-Z0-9]+\.[^\s]{2,})/
/(https?:\/\/(?:\.|(?!))[a-zA-Z0-9][a-zA-Z0-9-]+[a-zA-Z0-9]\.[^\s]{2,}|\.[a-zA-Z0-9][a-zA-Z0-9-]+[a-zA-Z0-9]\.[^\s]{2,}|https?:\/\/(?:\.|(?!))[a-zA-Z0-9]+\.[^\s]{2,}|\.[a-zA-Z0-9]+\.[^\s]{2,})/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that you may need to remove the \. that appeared after the www portions. Currently, a URL like http://.example.org/mrf.json will pass validation, but http://example.org/mrf.json will fail. Additionally, the expression should enforce a full string match with ^ at the start and $ at the end. Otherwise, a typo like htps://example-hospital.com could slip through.

I know you mentioned that you couldn't get the valid-url package to do what you wanted, but I think trying to build up a huge regex like this is going to be hard to test. What about it wasn't working well?

@eoverly
Copy link
Collaborator Author

eoverly commented Nov 7, 2024 via email

@eoverly eoverly merged commit 2a5cbbf into main Nov 20, 2024
4 checks passed
@eoverly
Copy link
Collaborator Author

eoverly commented Nov 20, 2024

Edited the url regex

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.

2 participants