Simplify StringStyle.update(part:) switch statement #381
Conversation
https://bugs.swift.org/browse/SR-2 was resolved in Swift 4.1. Removing support for Swift 4.0 is necessary to fix #345.
https://bugs.swift.org/browse/SR-2 was resolved in Swift 4.1. That allows simplifying the switch statement in StringStyle.update(part:) to remove nested switches, defaults, and fatal errors. Fixes #345.
|
I belieeeeve that’s correct, but I’d feel better if I could see a Swift bug # saying when it was changed. As for this update, it looks great, but I’m not sure I’m comfortable dropping Swift 4.0 support just yet. Xcode 11 won’t be required for App Store submissions until April of 2020, and even then, people may be using this for apps that are not in the App Store. The code is gross, yes, and your cleanup is really nice, but since the code itself is working and (mostly) well-tested, I see this as an ain’t-broke-don’t-fix-it scenario. If any of our assumptions about Xcode versions are wrong, or just given enough time, I think we could reconsider. |
|
Oh, and thanks for this contribution! The code itself looks really nice. It’s just complicated by some unfortunate historical/technical context. |
|
I should have linked directly to SR-2—it was closed with a comment that it was implemented in Swift 4.1. This was interesting to me, since swiftlang/swift#9457 was merged into Instead of increasing the Swift version requirement from 4.0 to 4.2, it would be possible to also support 4.1, which is included with Xcode 9.3. I can’t find the current minimum Xcode version for App Store submissions, but presumably it’s already Xcode 10. Still, you’re right that apps that aren’t on the App Store might be using BonMot with Xcode < 9.3. I also noticed that the readme badge mentions support for Swift 4.2 + 5.0. Should that include 4.0 too? |
|
Good call on the badge. I’m not sure if it’s helping or hurting more. 4.2 and 5.0 are the versions we’re testing with on CircleCI (see .circleci/config.yml if you want the details), so in that sense, they’re all that’s currently supported. But that doesn’t mean it doesn’t work in older versions; just that we’re not currently guaranteeing compatibility. No one has complained yet, so maybe we’re good? ¯\_(ツ)_/¯ |
|
Hey there! Happy one year since opening this PR. (And what a year.) I wanted to check in and see what your thoughts are on getting this ready to merge. No pressure either way, but I figure at this point we should either merge or close this PR. |
|
Hi! Yes, let's get this merged. Some stuff has changed on the build side of these things, so maybe you could rebase on latest upstream? That would kick off new builds. And I agree that the readme should be explicit about Swift 4.1+ or 4.2+ or whatever you deem appropriate based on the changes. |
| case let .hyphenationFactor(hyphenationFactor): | ||
| self.hyphenationFactor = hyphenationFactor | ||
| #if os(iOS) || os(tvOS) || os(watchOS) | ||
| case let .speaksPunctuation(speaksPunctuation): |
There was a problem hiding this comment.
I think I'd prefer to keep everything indented inside the #if blocks to make it easier to visually chunk this into its constituent parts.
There was a problem hiding this comment.
Okay, I changed it. I originally removed that indentation because the default Xcode indentation (highlight code, then control+I) doesn’t add extra indentation within #if blocks.
|
Oops, accidentally closed the PR. Reopened now. I rebased on I think it’s best to remove Swift 4.0 support and increase the minimum version to Swift 4.2, rather than adding new support for Swift 4.1. |
|
GitHub won’t let me reopen the PR because I force-pushed while it was closed. Huh. Let’s continue in #396. |
2 participants
Fixes #345 using the improved interaction between
switchand#ifin Swift 4.1+.I believe this requires removing support for Swift 4.0. Let me know if that assumption is incorrect.