-
Notifications
You must be signed in to change notification settings - Fork 128
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
Updated documentation in README to align with Swift 3 #64
Conversation
Do let me know if there's anything more I can do :) |
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.
Thanks a lot! I have a few notes but nothing major.
README.md
Outdated
@@ -19,24 +19,25 @@ the OAuth Flow | |||
You can initialize a new config for `github.com` as follows: | |||
|
|||
```swift | |||
let config = TokenConfiguration(token: "12345") | |||
let config = TokenConfiguration() |
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.
This should not have changed as it still works the "old way", see ConfigurationTests.swift
README.md
Outdated
@@ -49,8 +50,16 @@ user has to login to your application. This also handles the OAuth flow. | |||
You can authenticate an user for `github.com` as follows: | |||
|
|||
```swift | |||
let config = OAuthConfiguration(token: "<Your Client ID>", secret: "<Your Client secret>", scopes: ["repo", "read:org"]) |
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.
This should not have changed. It is also not recommended to put it into the application:didFinishLaunchingWithOptions:
method as this will launch your app and regardless of if your are already logged in or not, redirect you to the GitHub website.
I propose to revert this change.
README.md
Outdated
|
||
let _ = Octokit(tokenConfig).me() { response in | ||
switch response { | ||
case .success(let user): |
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.
I would prefer to leave the documentation like it is and just fix the compiler errors so lowercasing the enum and move to print
instead of println
README.md
Outdated
@@ -92,15 +99,18 @@ necessary to do the OAuth Flow again. You can just use a `TokenConfiguration`. | |||
|
|||
```swift | |||
let token = // get your token from your keychain, user defaults (not recommended) etc. | |||
let config = TokenConfiguration(token) | |||
Octokit(config).user("octocat") { response in | |||
let config = TokenConfiguration() |
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.
This should not have changed in Swift 3
README.md
Outdated
println(user.login) | ||
case .Failure(let error): | ||
println(error) | ||
case .success(let user): |
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.
Can you check the indentation here? Seems to me that these are indented too much
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.
Ok, all done.
Perfect, thanks a lot! |
Updated documentation in README to align with Swift 3
No description provided.