SemanticVersion icon indicating copy to clipboard operation
SemanticVersion copied to clipboard

Add support for modern Swift regex, when available

Open chriseplettsonos opened this issue 2 years ago • 5 comments

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.

chriseplettsonos avatar Nov 03 '23 14:11 chriseplettsonos

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?

finestructure avatar Nov 04 '23 13:11 finestructure

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),
    ],

chriseplettsonos avatar Nov 13 '23 14:11 chriseplettsonos

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 avatar Nov 14 '23 08:11 finestructure

@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?

chriseplettsonos avatar Nov 17 '23 14:11 chriseplettsonos

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!

finestructure avatar Nov 17 '23 14:11 finestructure