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

Feature - Prepping 3.1.0 Release #6

Merged
merged 7 commits into from
Apr 10, 2018
Merged

Conversation

cnoon
Copy link
Member

@cnoon cnoon commented Apr 10, 2018

This PR updates SQift to Xcode 9.3 and and fixes any compiler warnings from Swift 4.1. The change worth calling out here is the removal of the CodableBinding protocol extension explicitly setting the BindingType associated type to Data in a typealias. The Swift 4.1 compiler now complains that you shouldn't do that and should instead define the protocol extension with a generic constraint. See this Swift forum thread and SR-7324 for more info.

I'm a bit torn as to how we should release this change. The options I see are:

  1. Release this as a PATCH release and ignore the fact that the Swift 4.1 change requires us to make a non-backwards compatible change to get rid of a warning
  2. Release this as a MAJOR version release since we're required to make a non-backwards compatible change to silence the compiler warnings
  3. Release a PATCH release and leave the compiler warning in there. Then fix the compiler warning when we actually start working on SQift 4.x.

    It is important to note that SQift 4.x is not currently on our radar in any capacity.

Let's openly discuss these options in this PR to iron out the best way to release Swift 4.1 and Xcode 9.3 support.

@cnoon cnoon added this to the 3.0.2 milestone Apr 10, 2018
@cnoon cnoon self-assigned this Apr 10, 2018
/// The binding type of a parameter to bind to a statement.
public typealias BindingType = Data

extension CodableBinding where BindingType == Data {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the change worth calling out. The Swift 4.1 compiler throws a warning now when defining the typealias in the protocol or the protocol extension. And rightfully so, because it can lead to all sorts of odd behavior when redefining the typealias in conforming types or subclasses.

Copy link
Contributor

Choose a reason for hiding this comment

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

Read the thread and SR and the change seems like the right thing to do, but to make sure I'm grokking this... with the new syntax, this extension is only available when the BindingType type is Data, correct?

How does this break existing users of the code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great question. The only place this would break an existing user is if you had created a CodableBinding type such as a Person struct that you wanted to write directly into the database as a data blob. We actually have these in the test suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, makes sense. I'd suggest #1 as the version strategy. While this does require changes to the callers code, it's more of a slight language shift caused by Swift 4.1 than a "we actually changed the API" thing. This doesn't feel like a major version change to me.

@@ -17,6 +17,8 @@ class BindingTestCase: BaseTestCase {
// MARK: - Helper Types

private struct Person: CodableBinding, Equatable {
typealias BindingType = Data
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's an example @AtomicCat of the change clients would be required to make on Swift 4.1.

@cnoon cnoon changed the title Feature - Prepping 3.0.2 Release Feature - Prepping 3.1.0 Release Apr 10, 2018
@cnoon cnoon modified the milestones: 3.0.2, 3.1.0 Apr 10, 2018
@cnoon cnoon merged commit a937fa2 into master Apr 10, 2018
@cnoon cnoon deleted the feature/prepping-3.0.2-release branch April 10, 2018 18:32
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.

4 participants