feat(percent-encoding): add RFC2396 ascii sets #971
Conversation
There was a problem hiding this comment.
I'd suggest putting the add_range method and the transitions of the existing code to use it into its own PR, which can likely be merged fairly non-controversially as a good simplification.
I think it would be worth it to pull these out into a specific module (or possibly a crate) named after the RFC. There's another PR that's open to add the whatwg url definitions which are subtly different from the RFC definitions. #837. It would be good to have both these available for different circumstances. The RFC defined values are good for things defined by RFCs (backend / services etc.), while the whatwg versions are better for things more frontend / http focused.
BTW 2396 is updated by https://www.rfc-editor.org/rfc/rfc3986. The reason I came to look at this crate in the first place is that I wanted a spec compliant way to write a percent encoded query string for implementing another RFC which doesn't currently have a crate.
| AsciiSet { mask } | ||
| } | ||
|
|
||
| pub const fn add_range(&self, start: u8, end: u8) -> Self { |
There was a problem hiding this comment.
If this took Range instead of two vars, then it would be more obvious that this was the half-open range (it's actually the inclusive range, but that's not obvious from the method name at all)
| .add(b':') | ||
| pub const NON_ALPHANUMERIC: &AsciiSet = &ALPHA_NUM.complement(); | ||
|
|
||
| /// [RFC2396](https://datatracker.ietf.org/doc/html/rfc2396). |
There was a problem hiding this comment.
non-blocking nit:
| /// [RFC2396](https://datatracker.ietf.org/doc/html/rfc2396). | |
| /// Lower case alpha numeric characters defined in [RFC2396](https://datatracker.ietf.org/doc/html/rfc2396#section-1.6). |
A good description like this helps users searching for things (e.g. with a search engine), and the direct link to the section that these are defined in the RFC is useful when you need to dig into various things (obviously it's less useful for this particular case, but it's generally useful to have this as a general thing I think).
2 participants
I know the doc comment says that you intentionally don't have a lot of sets, but I feel like common ones should probably be supported if anyone using the library has to add them.
I guess if this is unwanted, I could make a companion library, but I'm not sure if I would maintain it in case there are ever breaking changes (and I would want the
add_rangeanyway).