-
Notifications
You must be signed in to change notification settings - Fork 26
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
RSC1: Add token string constructor. #158
Conversation
return | ||
} | ||
|
||
let client = ARTRest(token: tokenDetails.token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you just need a token you can use getTestToken
from test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not be initialising with just a token i.e. ARTRest(tokenDetails.token)
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ricardopereira Good point. Will do.
Sorry @mattheworiordan , I don't know what you mean. What you say is how it looks like now (except for the named parameter).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other languages we have an overloaded constructor so that you can pass in just a token or API key string, or options. If this is not possible in iOS then it's absolutely fine, but if it is possible it's a nice touch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be done, but it's strongly anti-idiomatic. All constructors that take arguments are like initWithSomething:something
. So we have initWithOptions
, initWithToken
, initWithKey
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Note that Swift automatically removes the "With", but still requires a named argument.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion removes the initWith
:
Objective-C initializers begin with init, or initWith: if the initializer takes one or more arguments. When an Objective-C initializer is imported by Swift, the init prefix becomes an init keyword to indicate that the method is a Swift initializer. If the initializer takes an argument, the With is removed and the rest of the selector is divided up into named parameters accordingly.
https://developer.apple.com/library/ios/documentation/Swift/Conceptual/BuildingCocoaApps/InteractingWithObjective-CAPIs.html#//apple_ref/doc/uid/TP40014216-CH4-ID35
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the designated initialiser can be used like ARTRest(value)
. Right now, I think the default initialiser is initWithLogger:andOptions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you just need a token you can use getTestToken from test suite.
Done at 3fd9a731ec4a1490c7c11ecbc9b22195f60ac1de
9345d4f
to
3fd9a73
Compare
3fd9a73
to
6d3e446
Compare
LGTM |
1 similar comment
LGTM |
RSC1: Add token string constructor.
No description provided.