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

Adjust _REG_NAME_PAT to reject invalid characters #2905

Closed
wants to merge 2 commits into from

Conversation

kenballus
Copy link
Contributor

Addresses #2802.

@kenballus
Copy link
Contributor Author

It might be preferable to percent-encode invalid characters instead of rejecting them. Feedback welcome!

@jayaddison
Copy link

If you have time @kenballus, there's another potential regex simplification that I've been wondering about within the same Python module. Perhaps we could trade each other some code review/inspection time?

@kenballus
Copy link
Contributor Author

If you have time @kenballus, there's another potential regex simplification that I've been wondering about within the same Python module. Perhaps we could trade each other some code review/inspection time?

I am definitely up for that. Shoot me an email and we can set up a time.
(My email is listed at the bottom of this page)

@jayaddison
Copy link

Despite my initial enthusiasm I haven't found the time to arrange a time yet, my apologies. A couple of questions that did occur to me in the meantime:

  • Could/should the rfc3986 library provide much of the validation here (perhaps even instead of some/all of the regexes currently in use)?
  • Is support for WHATWG URLs required here?

@kenballus
Copy link
Contributor Author

Could/should the rfc3986 library provide much of the validation here (perhaps even instead of some/all of the regexes currently in use)?

Unfortunately, that library has URL parsing/validating issues as well. See this issue.

Is support for WHATWG URLs required here?

Some parts of WHATWG are supported, like '\' to '/' translation, but my understanding is that the RFCs are prioritized over WHATWG in this library.

@jayaddison
Copy link

Thanks @kenballus, and sorry for the delays responding. I think I'm beyond my level of understanding on this one, so I'm going to step aside.

@sethmlarson
Copy link
Member

Closing this with justification here: #2802 (comment)

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.

None yet

3 participants