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

Fixed - Ensure existing refreshToken returned in Credentials Manager #146

Merged
merged 3 commits into from
Sep 7, 2017

Conversation

cocojoe
Copy link
Member

@cocojoe cocojoe commented Sep 5, 2017

Added - Renewed credentials are autosaved in Credentials Manager

Added - Renewed credentials are autosaved in Credentials Manager
@cocojoe
Copy link
Member Author

cocojoe commented Sep 5, 2017

#144

refreshToken: refreshToken,
expiresIn: credentials.expiresIn,
scope: credentials.scope)
_ = self.store(credentials: newCredentials)
Copy link
Contributor

Choose a reason for hiding this comment

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

what does the _ mean?? I can't find it as a declared variable in the class scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is to avoiding warnings, if I set it to a variable e.g. foo = self... Xcode would warn that it is not used. So you use _ =

Copy link
Contributor

Choose a reason for hiding this comment

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

Why set it anyway if it's not being used?

Copy link
Member Author

Choose a reason for hiding this comment

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

You get a warning if it's not set 😄

@@ -69,7 +69,6 @@ public class _ObjectiveManagementAPI: NSObject {
}

@objc(unlinkUserWithIdentifier:provider:fromUserId:callback:)
// swiftlint:disable:next function_parameter_count
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are all this lint checks removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

In latest Swiftlint 0.22 they are no longer required.

 Line 47: SwiftLint rule 'function_parameter_count' did not trigger a violation in the disabled region. Please remove the disable command.
[03:09:56]: ▸ /Users/distiller/Auth0.swift/Auth0/OAuth2Grant.swift

credentialsManager.credentials { error = $0; newCredentials = $1
expect(error).to(beNil())
credentialsManager.credentials {
expect($1!.accessToken) == NewAccessToken
Copy link
Contributor

Choose a reason for hiding this comment

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

here you use $1! but in the above test you refer to the same value as newCredentials. Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not the same value, the second credentialsManager.credentials { is another closure so the $0,$1 are different from the first closure.

credentials = Credentials(accessToken: AccessToken, tokenType: TokenType, idToken: IdToken, refreshToken: RefreshToken, expiresIn: Date(timeIntervalSinceNow: -3600))
_ = credentialsManager.store(credentials: credentials)
waitUntil(timeout: 2) { done in
credentialsManager.credentials { error = $0; newCredentials = $1
expect(error).to(beNil())
expect(newCredentials?.accessToken) == AccessToken
expect(newCredentials?.accessToken) == NewAccessToken
Copy link
Contributor

Choose a reason for hiding this comment

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

idToken will change too, as the dates change. You should test that too.

@cocojoe cocojoe force-pushed the added-credentials-manager-autosave branch from e757bc4 to 289f4e4 Compare September 7, 2017 14:11
lbalmaceda
lbalmaceda previously approved these changes Sep 7, 2017
@cocojoe cocojoe force-pushed the added-credentials-manager-autosave branch from 3aff521 to 0bc7f53 Compare September 7, 2017 18:29
@cocojoe cocojoe added this to the v1-Next milestone Sep 7, 2017
@cocojoe cocojoe merged commit a454345 into master Sep 7, 2017
@cocojoe cocojoe deleted the added-credentials-manager-autosave branch September 7, 2017 19:12
@cocojoe cocojoe modified the milestones: 1.7.2, v1-Next Sep 11, 2017
@cocojoe cocojoe changed the title Fix - Ensure existing refreshToken returned in Credentials Manager Fixed - Ensure existing refreshToken returned in Credentials Manager Sep 11, 2017
@cocojoe cocojoe mentioned this pull request Sep 11, 2017
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.

None yet

2 participants