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

Renaming a few Swift 3 methods #3874

Closed
wants to merge 1 commit into from
Closed

Renaming a few Swift 3 methods #3874

wants to merge 1 commit into from

Conversation

austinzheng
Copy link
Contributor

@bdash @JadenGeller @jpsim @tgoyne

Changes:

  • 'createObject(ofType:populatedWith:)' is now 'createObject(_:from:)'
  • 'createDynamicObject(ofType:populatedWith:)' is now 'createDynamicObject(ofType:from:)'
  • 'allObjects(ofType:)' is now 'objects(ofType:)'
  • 'allDynamicObjects(ofType:)' is now 'dynamicObjects(ofType:)'

@JadenGeller
Copy link
Contributor

I understand that ofType: is necessary for dynamic objects because of the String argument and not necessary for normal objects, but I'd prefer consistency over slightly shortening the call site of createObject.

@JadenGeller
Copy link
Contributor

JadenGeller commented Jul 14, 2016

Have we ever considered doing some sort of user testing to determine optimal API naming? I wonder if allObjects(_:) is sufficiently more clear than objects(_:) that it's worth the verbose API. Or maybe retrieveObjects(_:) would be more understandable to users.

@austinzheng
Copy link
Contributor Author

I'd also rather keep ofType: in the methods, but I guess I'll wait for some more folks to chime in.

@JadenGeller
Copy link
Contributor

If we're going to inconsistently use ofType:, we should at least only use it on String arguments.

@JadenGeller
Copy link
Contributor

JadenGeller commented Jul 14, 2016

Maybe this is crazy, but thoughts on

func create<T>(_ type: T.Type, from: AnyObject) -> T
func create(_ type: DynamicType, from: AnyObject) -> DynamicObject

where DynamicType is a struct wrapping a String?

It results in both simpler API naming for the common case and similar methods sharing a base name.


As an alternative, we could use the object schema instead of the type name.

func create(_ type: RLMObjectSchema, from: AnyObject) -> DynamicObject

This helps users avoid typos when using the dynamic API since they'll likely keep around a variable representing the type.

let foo = realm.schema["foo"]!
realm.create(foo, from: json)

This would leave us with the following, more concise API.

create(_:from:)
objects(_:)

@bdash
Copy link
Contributor

bdash commented Jul 15, 2016

objects(…), rather than allObjects(…), is consistent with the naming of Dictionary.keys / Dictionary.values in Swift. The Objective-C equivalents on NSDictionary have the "all" prefix but they're absent from the Swift API.

@bdash
Copy link
Contributor

bdash commented Jul 15, 2016

Should this change also make the label name of Object(value:) consistent with the label used in Realm.create?

@austinzheng
Copy link
Contributor Author

@bdash Good catch on Object(value:); they should all be consistent.

Would anyone object if I restored ofType: to the APIs?

@JadenGeller A struct wrapper is too heavy IMO, but a typealias would be almost as good (wouldn't stop you from doing stupid things, but it would let you see the intended purpose of the string at a glance).

@bdash
Copy link
Contributor

bdash commented Jul 15, 2016

Is realm.createObject(ofType: Person.self, from: […]) really so much clearer than realm.create(Person.self, from: […]) that it justifies the extra overhead? "Create object of type person" doesn't seem like a big improvement over "create person" to me.

@austinzheng
Copy link
Contributor Author

austinzheng commented Jul 15, 2016

It's not, but then you have objects(Foo.self) instead of objects(ofType: Foo.self), which I think is a regression. But maybe not? Or maybe we can just keep ofType: for one but not the other? It wouldn't be a disaster if we got rid of ofType: altogether.

@bdash
Copy link
Contributor

bdash commented Jul 15, 2016

Realm.objects(Person.self), while what we currently have for Swift 2, doesn't read as naturally as some of our other method names. Including ofType: does help in this case. We could potentially omit it if we changed objects for a method name like fetch or all, but then the fact that type names are singular nouns starts feeling awkward (Realm.all(Person.self)).

@austinzheng
Copy link
Contributor Author

Maybe we should keep ofType: if the function name is a noun (e.g. objects()), and strike it if the function name is a verb (in which case the type is the implied grammatical direct object: "Realm, create a Person"). I think that would give us both consistency and a good user experience.

@JadenGeller
Copy link
Contributor

@austinzheng A typealias could be nice, but not a replacement for the argument label. Did you have thoughts on using an RLMObjectSchema instead of a String to specify the type?

@piv199
Copy link

piv199 commented Jul 24, 2016

Well, as of Swift3 createObject should be replaced with create.

Changes:
- 'createObject(ofType:populatedWith:)' is now 'create(_:from:)'
- 'createDynamicObject(ofType:populatedWith:)' is now 'createDynamicObject(ofType:from:)'
- 'allObjects(ofType:)' is now 'objects(ofType:)'
- 'allDynamicObjects(ofType:)' is now 'dynamicObjects(ofType:)'
@austinzheng
Copy link
Contributor Author

@bdash @JadenGeller Changed createObject(...) to create(...).

@austinzheng
Copy link
Contributor Author

@JadenGeller using the object schema instead of the string is an interesting idea, but should probably be discussed and implemented separately.

@austinzheng
Copy link
Contributor Author

Are these renamings still something we want to pursue? If so I'll rebase and merge, after making any further improvements that you might suggest.

@austinzheng austinzheng self-assigned this Aug 15, 2016
@jpsim
Copy link
Contributor

jpsim commented Aug 15, 2016

This is still something that needs to happen, and the churn of the language is one reason I've been staying away from this, though supposedly now it's mostly settled?

@JadenGeller
Copy link
Contributor

I think it has mostly settled.

@austinzheng austinzheng deleted the az-api2 branch September 14, 2016 23:08
@austinzheng
Copy link
Contributor Author

Closed since the API changes are being made in #4078.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants