Skip to content

Conversation

@jleandroperez
Copy link
Contributor

@jleandroperez jleandroperez commented Jun 25, 2018

Details:

  • Adds missing documentation in StoresManager
  • Simplifies StoresManagerState Protocol API
  • Relocates AuthenticatedState and DeauthenticatedState to their own files
  • Implements StoresManagerTests

Closes #93

Testing:

Please verify that the new (and shiny!) unit tests look green!

@jleandroperez jleandroperez added the type: task An internally driven task. label Jun 25, 2018
@jleandroperez jleandroperez self-assigned this Jun 25, 2018
@jleandroperez jleandroperez changed the title Issue/93 stores manager improvements StoresManager: API Improvements Jun 25, 2018
Copy link
Contributor

@mindgraffiti mindgraffiti left a comment

Choose a reason for hiding this comment

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

Love this! Thank you for splitting out the authenticator and de-authenticator into separate files and separating them from the StoresManagerState protocol.

All unit tests ✅
2 nitpick-y items, 2 general comments, then :shipit:



/// Designated Initializer
///
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete repeated method documentation comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that!!

///
private init() { }
var isAuthenticated: Bool {
return state is AuthenticatedState
Copy link
Contributor

Choose a reason for hiding this comment

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

hey I just reviewed this concept last week! is is the Swift way of checking type. It's much more elegant than saying[thisThing isKindOfClass: [MyThing class]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, it's the same thing!

/// Shared Instance
///
private static var state: StoresManagerState = initialState() {
static var shared = StoresManager(keychain: .shared)
Copy link
Contributor

Choose a reason for hiding this comment

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

I spy the first Singleton in our app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... it was either this, or AppDelegate.shared.storesManager!!! =(

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not against Singletons :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhhh but i am!! 😄

private var manager: StoresManager!


// MARK: - Overriden Methods
Copy link
Contributor

Choose a reason for hiding this comment

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

Overridden instead of Overriden

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up!!

@jleandroperez
Copy link
Contributor Author

@mindgraffiti ready for another peek! thanks in advance!!

Copy link
Contributor

@mindgraffiti mindgraffiti left a comment

Choose a reason for hiding this comment

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

:shipit: sir!

@jleandroperez
Copy link
Contributor Author

Thank you Thuy!!

@jleandroperez jleandroperez merged commit 6f20e78 into develop Jun 28, 2018
@jleandroperez jleandroperez deleted the issue/93-stores-manager-improvements branch June 28, 2018 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants