-
Notifications
You must be signed in to change notification settings - Fork 325
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
Add AsciiSet::EMPTY and boolean operators #969
Conversation
In RFCs, the sets of characters to percent-encode are often defined as the union of multiple sets. This change adds an `EMPTY` constant to `AsciiSet` and implements the `Add` trait for `AsciiSet` so that sets can be combined with the `+` operator. AsciiSet now derives `Debug`, `PartialEq`, and `Eq` so that it can be used in tests.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #969 +/- ##
=======================================
Coverage ? 81.85%
=======================================
Files ? 21
Lines ? 3560
Branches ? 0
=======================================
Hits ? 2914
Misses ? 646
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Thanks! |
@@ -77,6 +78,11 @@ const ASCII_RANGE_LEN: usize = 0x80; | |||
const BITS_PER_CHUNK: usize = 8 * mem::size_of::<Chunk>(); | |||
|
|||
impl AsciiSet { | |||
/// An empty set. | |||
pub const EMPTY: AsciiSet = AsciiSet { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it's now inconsistent with the existing constants and functions taking &'static AsciiSet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wasn't 100% sure about that. I went with EMPTY being a constant on the AsciiSet as empty seems like an inherent property of a type, but the other constants seem like usages of AsciiSet. I was 70/30% on this being right, so wouldn't object to this being changed to be consistent with the other constants.
The rationale for making the constants references rather than just values all seemed odd to me. What was that necessary for?
Edit: let's disuss on #970 instead of here.
In RFCs, the sets of characters to percent-encode are often defined as
the union of multiple sets. This change adds an
EMPTY
constant toAsciiSet
and implements theAdd
trait forAsciiSet
so that setscan be combined with the
+
operator. The ! negation operator is alsodefined, as well as equivalent constant functions for these (
union()
,complement()
).AsciiSet now derives
Debug
,PartialEq
, andEq
so that it can beused in tests.
Example: https://www.rfc-editor.org/rfc/rfc3986#section-3.4 defines
Using this new method, this can be easily represented as:
Similarly the set of characters that must be encoded is defined as the set of characters that are not in the allowed characters
https://www.rfc-editor.org/rfc/rfc3986#section-2.2
So a part like query is defined in https://www.rfc-editor.org/rfc/rfc3986#appendix-A as:
which can be translated to: