Skip to content

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants