Skip to content
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 support for modern Swift regex, when available #20

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chriseplettsonos
Copy link
Contributor

This change makes it so, if conditions are met (i.e. - swift version and OS version are capable), the library will use Swift regex instead of NSRegularExpression.

@cla-bot cla-bot bot added the cla-signed label Nov 3, 2023
@finestructure
Copy link
Member

Thanks for the PR, @chriseplettsonos . I like the new regex format but I think keeping two implementations around is making this quite a bit more complicated than necessary. I think we should keep this change around and move to it when the package becomes Swift 5.7+ only.

I've just prepared a PR to bump it to 5.6+ (which is the range we test for in SPI).

When 5.10 comes out we'll bump SPI testing to 5.7+ and at that point we can make SemanticVersion 5.7+ as well.

Does that make sense?

@chriseplettsonos
Copy link
Contributor Author

chriseplettsonos commented Nov 13, 2023

Thanks for the PR, @chriseplettsonos . I like the new regex format but I think keeping two implementations around is making this quite a bit more complicated than necessary. I think we should keep this change around and move to it when the package becomes Swift 5.7+ only.

I've just prepared a PR to bump it to 5.6+ (which is the range we test for in SPI).

When 5.10 comes out we'll bump SPI testing to 5.7+ and at that point we can make SemanticVersion 5.7+ as well.

Does that make sense?

Yeah, that's fine. All our stuff is on 5.9 (and only runs on more recent OS versions) now, so I get hives when I see NSRegularExpression ;)

Keep in mind it's not just a matter of switching to Swift 5.7, but also bumping the minimum OS versions, as the new regex stuff requires newer runtimes. You'd have to set the min OS versions in Package.swift, otherwise you'd need to keep the NSRegularExpression implementation around. I'm assuming that on linux, the runtime is part of the swift toolchain, and so there's no min OS requirement there…

I'm not sure if you're OK with setting these requirements on the entire package.

    platforms: [
        .iOS(.v16),
        .macOS(.v13),
        .watchOS(.v9),
        .tvOS(.v16),
    ],

@finestructure
Copy link
Member

I'm not sure if you're OK with setting these requirements on the entire package.

    platforms: [
        .iOS(.v16),
        .macOS(.v13),
        .watchOS(.v9),
        .tvOS(.v16),
    ],

Good point, that should be fine. I'll test it in the places where we use SemanticVersion to make sure we're not running into issues!

Marking this as "draft" for now so it isn't merged accidentally.

Thanks again for the PR!

@finestructure finestructure marked this pull request as draft November 14, 2023 08:04
@chriseplettsonos
Copy link
Contributor Author

@finestructure, do you want me to update this draft PR to make all the version bumps (swift toolchain, OS versions) and remove the conditional compilation and execution?

@finestructure
Copy link
Member

That's a good idea, we could prep it so it's ready for merge once Swift 5.10 comes out. The only downside is that it might need rebasing in the meantime. Although I don't envision lots of changes happening until then. Up to you really!

* Set minimum swift tools to 5.7
* Set appropriate min platform versions
* Remove all NSRegularExpression code
* Eliminate condtional compilation and @available checks, using Swift regex at all times
Copy link
Member

@finestructure finestructure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants