Skip to content
This repository was archived by the owner on Mar 12, 2024. It is now read-only.

Askcredential - #21

Merged
AliSoftware merged 6 commits into
masterfrom
askcredential
May 30, 2017
Merged

AliSoftware merged 6 commits into
masterfrom
askcredential

Conversation

@pierre-duchene

Copy link
Copy Markdown
Contributor

Add high level struct for ask credential functionality.
When renew token fail, NetworkStack need to ask caller how to fetch new credentials.

@AliSoftware

Copy link
Copy Markdown
Contributor

Is it possible to resolve the conflicts first? (You can do a rebase to make the git history clean)

@AliSoftware AliSoftware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should do the same for RenewTokenHandler. I see 2 solutions:

  • Have a struct for each (generalise the AskCredential struct to give it more generic names, and use an instance of that struct for askCredential and another instance of the same struct for renewToken)
  • Have a bigger struct containing the 4 closures and thus grouping informations for both the renewToken and the askCredential at once. This could allow sharing the same triggerCondition for both easily, by providing a convenience constructor taking one triggerCondition and using it for both, for example.

Comment thread Sources/AskCredential.swift Outdated
let triggerCondition = { (error: Error) -> Bool in
return AskCredential.shouldAskCredential(forError: error)
}
return triggerCondition

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should be able to just return AskCredential.shouldAskCredential(forError:) I think (the closure and the function have the same signature)

So in fact you can merge those two private funcs into one

@AliSoftware
AliSoftware merged commit 543c089 into master May 30, 2017
@AliSoftware

Copy link
Copy Markdown
Contributor

Don't forget to add an issue so we think about implementing #21 (review) later

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.

3 participants