-
Notifications
You must be signed in to change notification settings - Fork 55
Make "swift-" prefix optional in MainSnapshotParser #240
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
Make "swift-" prefix optional in MainSnapshotParser #240
Conversation
Existing tools like `swiftenv` use `DEVELOPMENT-SNAPSHOT-YYYY-mm-dd-a` for main snapshots, without the `swift-` prefix. The `MainSnapshotParser` should be able to handle this and this change mirrors the `ReleaseSnapshotParser` in this regard, which already supports both `swift-` prefixed and unprefixed versions.
/// - main-snapshot | ||
/// - main-SNAPSHOT-YYYY-mm-dd | ||
/// - main-SNAPSHOT | ||
/// - swift-DEVELOPMENT-SNAPSHOT-YYYY-mm-dd-a |
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.
question: Since the swift prefix is optional with this change, should the examples with the swift prefix remain here to indicate that these are valid selectors?
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 made this example style aligned with ReleaseSnapshotParser's comment, but just changed to include them.
struct MainSnapshotParser: ToolchainSelectorParser { | ||
static let regex: Regex<(Substring, Substring?)> = | ||
try! Regex("^(?:main-snapshot|swift-DEVELOPMENT-SNAPSHOT|main-SNAPSHOT)(?:-([0-9]{4}-[0-9]{2}-[0-9]{2}))?(?:-a)?$") | ||
try! Regex("^(?:swift-)?(?:main-snapshot|DEVELOPMENT-SNAPSHOT|main-SNAPSHOT)(?:-([0-9]{4}-[0-9]{2}-[0-9]{2}))?(?:-a)?$") |
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.
question: Does this regex now permit selections like swift-main-snapshot-YYYY-mm-dd-a
? Does this make sense as a potential selector?
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.
Yes, it's intentional. Given that we already accept swift-a.b-snapshot-YYYY-mm-dd
, it makes main
and release channel selectors more consistent.
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.
Thank you for your contribution. Overall this looks good. I've added a couple of questions.
@swift-ci test macOS |
@swift-ci test macOS |
Thanks again @kateinoigakukun for your contribution. |
Existing tools like
swiftenv
useDEVELOPMENT-SNAPSHOT-YYYY-mm-dd-a
for main snapshots, without theswift-
prefix. TheMainSnapshotParser
should be able to handle this and this change mirrors theReleaseSnapshotParser
in this regard, which already supports bothswift-
prefixed and unprefixed versions.