Review diffs between Swift 2 --> preliminary Swift 3 API #4078

austinzheng · 2016-09-12T17:10:42Z

@jpsim @bdash @tgoyne @stel @mrackwitz @kishikawakatsumi @TimOliver @JadenGeller

bdash · 2016-09-12T17:14:37Z

docs/swift-3-api.diff

+-    public func indexOf(predicate: NSPredicate) -> Int?
+-    public func indexOf(predicateFormat: String, _ args: AnyObject...) -> Int?
+    public func index(of object: T) -> Int?
+    public func indexOfObject(for predicate: NSPredicate) -> Int?


for feels odd here. It reads as "index of object for predicate". Something like "index of object matching predicate" would be clearer.

I'd propose index(of object: T) and index(matching predicate: NSPredicate).

jpsim · 2016-09-13T01:44:50Z

I have some recommendations: az/swift3apireview...jp/swift3apireview

I can make those edits directly on this branch if that's ok with @austinzheng.

austinzheng · 2016-09-13T15:04:01Z

@jpsim That's absolutely fine with me. Same with anyone else who wants to push suggestions as additional commits.

jpsim · 2016-09-13T15:07:37Z

@austinzheng merged, thanks.

jpsim · 2016-09-13T15:55:26Z

I want to start applying these changes later today, so I encourage anyone who still has feedback to give on these APIs to do that soon.

tgoyne · 2016-09-13T17:40:14Z

Everything looks reasonable to me now.

austinzheng · 2016-09-13T18:03:57Z

Looks good to me as well.

Should autorefresh (a boolean property) be something like willAutorefresh or isAutorefreshing?

jpsim · 2016-09-13T18:31:59Z

Should autorefresh (a boolean property) be something like willAutorefresh or isAutorefreshing?

I'd say no, due to those being the wrong words. "Automatically Refresh" already accurately represents the behavior, but not worth expanding from the current shortened form of autorefresh. Given that we can't change the Objective-C property name, I'd prefer aligning the Swift one too.

jpsim · 2016-09-13T19:42:44Z

I've applied the changes from the diff to our APIs.

jpsim · 2016-09-13T21:27:15Z

2 CI failures are due to Abort trap 6 when running xcodebuild and Application "io.realm.TestHost" is unknown to FrontBoard. in CoreSimulator.log.

The first one we could add to the "known Xcode error" detection in build.sh to rerun the job, the second one seems like something that could fixed by adapting reset-simulators.sh to launch the same simulator device as the job will use I think...

jpsim · 2016-09-14T01:01:20Z

I'd like to merge this tomorrow, so I'd appreciate a quick review.

You'll find the completes docs diff with 1.0.2 & Swift 2 here: https://gist.github.com/jpsim/87002160be20193eb4b85a2ea1905231

Realm Swift 1.0.2 Swift 2.2 docs here: https://realm.io/docs/swift/latest/api
Realm Swift master Swift 3.0 docs here: https://static.realm.io/jazzy_demo/RealmSwift3

Looks like Object isn't documented at all for Swift 3, which I'll look into now.

austinzheng · 2016-09-14T01:03:27Z

I think Jazzy doesn't (yet) know how to deal with declarations marked with open.

jpsim · 2016-09-14T01:05:50Z

I think Jazzy doesn't (yet) know how to deal with declarations marked with open.

I think you're right. It has partial support for it, but not complete.

austinzheng · 2016-09-14T01:07:41Z

👍 , based on the diff changes you made.

Should we remove the .diff file from the final PR?

JadenGeller · 2016-09-14T01:09:52Z

RealmSwift/Realm.swift

@@ -300,7 +300,7 @@ public final class Realm {
    :nodoc:
    */
    @discardableResult
-    public func createDynamicObject(ofType typeName: String, populatedWith value: Any = [:], update: Bool = false) -> DynamicObject {
+    public func dynamicCreate(_ typeName: String, value: Any = [:], update: Bool = false) -> DynamicObject {


It might be nice for this to share a prefix with create. For example, create(typeNamed:value:update:).

This function's name starts with dynamic to prevent it from showing up in autocorrect since it's not what users want 99% of the time. Ideally we'd have Clang submodules in Swift and expose it in there, but alas we don't, so we namespace it like this.

jpsim · 2016-09-14T01:16:48Z

Should we remove the .diff file from the final PR?

Yes of course. I was waiting for this to pass review before doing that so that we can double check that my changes are correct based on our discussions around the .diff.

I think Jazzy doesn't (yet) know how to deal with declarations marked with open.

I think you're right. It has partial support for it, but not complete.

This was an easy fix in jazzy (realm/jazzy#655). I've updated the gist and API docs I linked to above.

jpsim · 2016-09-14T04:16:51Z

@bdash is the change you made in 0d7e7a1 to make RealmSwift.Error a struct still necessary? It still seems better suited as an enum to me, and the fact that it's a struct with its members only documented indirectly via its Code equivalent member complicates the docs.

See http://static.realm.io/jazzy_demo/RealmSwift3/Structs/Error.html

Unless I'm missing something, I suggest we switch that back to an enum.

austinzheng · 2016-09-14T04:21:52Z

If we want to not make adding new error enum cases a breaking change, it needs to be a struct. Otherwise, it can be an enum.

jpsim · 2016-09-14T04:24:32Z

If we want to not make adding new error enum cases a breaking change, it needs to be a struct. Otherwise, it can be an enum.

Good point. It'd still be nice to improve how the documentation for this is generated. Maybe moving the doc strings onto the Error members directly rather than the Code would be doable?

Fixes #4086

realm-ci · 2016-09-14T17:54:27Z

Can't set status; build failed.

Changes: - Documentation for Swift 3 APIs is now correct - Fixed an inconsistency where some APIs were `sorted(property:...)` rather than `sorted(byProperty:...)` - All Swift 3 enum case members are lowerCamelCase now

Fixing documentation for Swift 3 APIs

jpsim · 2016-09-15T18:55:38Z

@austinzheng @tgoyne @bdash I think this is ready to land.

tgoyne

👍

austinzheng added the S:Review label Sep 12, 2016

austinzheng self-assigned this Sep 12, 2016

bdash reviewed Sep 12, 2016
View reviewed changes

jpsim force-pushed the az/swift3apireview branch from 525d65f to c4ee14b Compare September 13, 2016 20:40

JadenGeller reviewed Sep 14, 2016
View reviewed changes

Austin Zheng and others added 4 commits September 13, 2016 18:27

Review diffs between Swift 2 --> preliminary Swift 3 API

e95cab8

edits to Austin's Swift 3 API review

7322bb5

byProperty & ofProperty

89d021e

add a space in @available(_,_,renamed:)

f564858

jpsim added 2 commits September 13, 2016 22:48

use ruby instead of jq to parse json from simctl in reset-simulators.sh

6c91feb

avoid simctl switch crash and avoid logging pid of springboard

6a37c83

jpsim force-pushed the az/swift3apireview branch from 3da08b7 to 6a37c83 Compare September 14, 2016 05:48

jpsim added 2 commits September 14, 2016 10:46

fix Error docs, re-add Equatable conformance and ~= operator

e3ae209

nest Notification enum inside Realm

b673078

Fixes #4086

add "Failed to lookup" simulator error to build.sh detection

9c55b7e

jpsim mentioned this pull request Sep 14, 2016

Realm notification enumname #4086

Closed

Austin Zheng and others added 5 commits September 14, 2016 12:48

Fixing documentation for Swift 3 APIs

a9571a6

Changes: - Documentation for Swift 3 APIs is now correct - Fixed an inconsistency where some APIs were `sorted(property:...)` rather than `sorted(byProperty:...)` - All Swift 3 enum case members are lowerCamelCase now

Remove accidental Swift 2 changes

124a5fa

Update sample projects

b75424a

Fix issues reported by swiftlint

6ab2ee9

Merge pull request #3829 from realm/az-docs

2602e6b

Fixing documentation for Swift 3 APIs

austinzheng assigned austinzheng and jpsim and unassigned austinzheng Sep 14, 2016

austinzheng mentioned this pull request Sep 14, 2016

Renaming a few Swift 3 methods #3874

Closed

jpsim added 2 commits September 15, 2016 11:06

remove API .diff

70a1276

fix CoreSimulator.log known Xcode error detection

1f49449

tgoyne approved these changes Sep 15, 2016

View reviewed changes

jpsim merged commit 41db543 into master Sep 15, 2016

jpsim deleted the az/swift3apireview branch September 15, 2016 21:29

jpsim removed the S:Review label Sep 15, 2016

liamnichols mentioned this pull request Sep 18, 2019

Add support for suppressing notifications using closure-based write/transaction methods #6252

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review diffs between Swift 2 --> preliminary Swift 3 API #4078

Review diffs between Swift 2 --> preliminary Swift 3 API #4078

austinzheng commented Sep 12, 2016

bdash Sep 12, 2016

jpsim Sep 13, 2016

jpsim commented Sep 13, 2016

austinzheng commented Sep 13, 2016 •

edited

Loading

jpsim commented Sep 13, 2016

jpsim commented Sep 13, 2016

tgoyne commented Sep 13, 2016

austinzheng commented Sep 13, 2016

jpsim commented Sep 13, 2016

jpsim commented Sep 13, 2016

jpsim commented Sep 13, 2016

jpsim commented Sep 14, 2016

austinzheng commented Sep 14, 2016

jpsim commented Sep 14, 2016

austinzheng commented Sep 14, 2016

JadenGeller Sep 14, 2016

jpsim Sep 14, 2016

jpsim commented Sep 14, 2016 •

edited

Loading

jpsim commented Sep 14, 2016

austinzheng commented Sep 14, 2016

jpsim commented Sep 14, 2016

realm-ci commented Sep 14, 2016

jpsim commented Sep 15, 2016

tgoyne left a comment

Review diffs between Swift 2 --> preliminary Swift 3 API #4078

Review diffs between Swift 2 --> preliminary Swift 3 API #4078

Conversation

austinzheng commented Sep 12, 2016

bdash Sep 12, 2016

Choose a reason for hiding this comment

jpsim Sep 13, 2016

Choose a reason for hiding this comment

jpsim commented Sep 13, 2016

austinzheng commented Sep 13, 2016 • edited Loading

jpsim commented Sep 13, 2016

jpsim commented Sep 13, 2016

tgoyne commented Sep 13, 2016

austinzheng commented Sep 13, 2016

jpsim commented Sep 13, 2016

jpsim commented Sep 13, 2016

jpsim commented Sep 13, 2016

jpsim commented Sep 14, 2016

austinzheng commented Sep 14, 2016

jpsim commented Sep 14, 2016

austinzheng commented Sep 14, 2016

JadenGeller Sep 14, 2016

Choose a reason for hiding this comment

jpsim Sep 14, 2016

Choose a reason for hiding this comment

jpsim commented Sep 14, 2016 • edited Loading

jpsim commented Sep 14, 2016

austinzheng commented Sep 14, 2016

jpsim commented Sep 14, 2016

realm-ci commented Sep 14, 2016

jpsim commented Sep 15, 2016

tgoyne left a comment

Choose a reason for hiding this comment

austinzheng commented Sep 13, 2016 •

edited

Loading

jpsim commented Sep 14, 2016 •

edited

Loading