-
Notifications
You must be signed in to change notification settings - Fork 2.4k
[SE-0351] Revise regex builder proposal #1634
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
Conversation
e02cfbf
to
f73b130
Compare
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.
Quick review of the first 2/3rds of this PR. Sorry it's not more in-depth, but I wanted to get the feedback out early
``` | ||
|
||
By conforming standard library types to `RegexComponent`, we allow them to be used inside the regex builder DSL as a match target. | ||
Note: | ||
- `RegexComponent` and `Regex`'s conformance to `RegexComponent` are available without importing `RegexBuilder`. All other types and conformances introduced in this proposal are in the `RegexBuilder` module. |
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.
Is this where we landed, with the String
conformance not being in the stdlib proper (i.e. Swift
)? I thought we were leaning the other way but I'll have to crawl over conversation threads to see.
Either way, let's be very explicit what we mean by "other types". Perhaps pointing out String
but also b, c, and d.
There's also a very real possibility of CharacterClass
getting pulled into the Swift
module at some point, perhaps even during the Unicode review.
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.
Is this where we landed, with the
String
conformance not being in the stdlib proper (i.e.Swift
)?
Yes.
Either way, let's be very explicit what we mean by "other types".
"All other types" seems pretty clear to me. The sentence below it already says explicitly:
By conforming standard library types to
RegexComponent
, we allow them to .... These conformances are available in theRegexBuilder
module.
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.
I think this reads really well now (including Michael's comments). I'm super excited for this!
Argh, Github comment threads are so hard to see over time. The remaining point is:
Is the intent to trap if that branch of an alternation wasn't taken, or to trap if the reference wasn't used with the regex? edit: to clarify, "ever being captured" could be confused as meaning it was set to a value instead of Maybe we can define "regex compilation time" as a term and use that to clarify? |
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.
Temporarily marking this as requesting changes concerning the trapping behavior of reference subscript and some other concerns. Don't want this to be merged accidentally.
Revision: - Capture takes throwing closures. (swiftlang/swift-experimental-string-processing#261) - Rename `Output` associated type to `RegexOutput`. (swiftlang/swift-experimental-string-processing#281) - Define primary associated type for `RegexComponent`. Clarification: - Clarify that `RegexComponent` and `Regex: RegexComponent` will be in the stdlib, not in `RegexBuilder`. - Make detailed design driven by example and move API definition to a collapsible. This is so that the complex result builder machinery won't obscure the API design. - Add alternative considered section about unifying `Capture` and `TryCapture`.
/// An anchor that matches at the first position of a match in the input | ||
/// string. | ||
/// | ||
/// This anchor is equivalent to `\y` in regex syntax. |
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 anchor is equivalent to `\y` in regex syntax. | |
/// This anchor is equivalent to `\G` in regex syntax. |
IIRC
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.
\y
is correct according to @natecook1000
Add a section for `mapOutput` method
} | ||
}.map { _, c1, c2, c3 in | ||
SemanticVersion(major: c1, minor: c2, patch: c3 ?? 0) | ||
} |
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 is such a compelling use
In the "Future directions" section, two instances of At the end of the document, [Regex Syntax]: https://github.com/apple/swift-evolution/blob/main/proposals/0355-regex-syntax-run-time-construction.md
[String Processing Algorithms]: https://github.com/apple/swift-evolution/blob/main/proposals/0357-regex-string-processing-algorithms.md |
`buildEither` was removed from the regex builder DSL proposal. See swiftlang/swift-evolution#1634.
Revision:
RegexComponent
.Clarification:
RegexComponent
andRegex: RegexComponent
will be in the stdlib, not inRegexBuilder
.Capture
andTryCapture
.