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

Improve URI.register_scheme tests and automatically upcase the given scheme #28

Merged
merged 1 commit into from
May 12, 2022

Conversation

eregon
Copy link
Member

@eregon eregon commented Jul 28, 2021

Follow-up of #27 (comment)
cc @byroot / @casperisfine and @hsbt

Maybe +/-/. could be supported by replacing them with some other characters valid as constant names, but not sure how to avoid conflicts (replacing all 3 with just _ seems bad), and whether someone would actually want a custom URI::Generic subclass for a scheme. Also any extra replacement to .upcase will make URI.for slower for non-initial schemes.

@eregon eregon force-pushed the improve-register_scheme-tests branch from 63b6cf0 to f9454d7 Compare July 28, 2021 10:07
@byroot
Copy link
Member

byroot commented Jul 28, 2021

Maybe +/-/. could be supported by replacing them with some other characters valid as constant names, but not sure how to avoid conflicts

Yeah, I almost went with some form of gsub('-', '__DASH__') etc, but I wasn't sure the overhead was worth it.

@eregon
Copy link
Member Author

eregon commented Jul 28, 2021

Indeed, and we can always expand in the future if someone requests it, while we can't remove it if we add it now.
The previous "API" using @@schemes wasn't really a proper public API so I think it's OK to have additional restrictions for URI.register_scheme.

@byroot
Copy link
Member

byroot commented Jul 28, 2021

using @@schemes wasn't really a proper public API so I think it's OK to have additional restrictions for URI.register_scheme.

Agreed. It was pretty much a monkey patch method before, so if people have really complex need they can extend .for.

@nurse
Copy link
Member

nurse commented May 12, 2022

@eregon Sorry about the late review but it looks good. Once you solve the conflicts, I'll merge this

…scheme

* Also add docs and mention current limitations.
* For reference, https://stackoverflow.com/a/3641782/388803 mentions the
  valid characters in schemes.
@eregon eregon force-pushed the improve-register_scheme-tests branch from f9454d7 to 2890342 Compare May 12, 2022 09:16
@eregon eregon merged commit 4346daa into ruby:master May 12, 2022
@eregon eregon deleted the improve-register_scheme-tests branch May 12, 2022 09:19
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.

3 participants