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

Migrate Cache version #647

Merged
merged 2 commits into from
Jun 9, 2017
Merged

Migrate Cache version #647

merged 2 commits into from
Jun 9, 2017

Conversation

zenangst
Copy link
Owner

@zenangst zenangst commented Jun 9, 2017

This migrates the StateCache to use the latest version of Cache.

@mention-bot
Copy link

@zenangst, thanks for your PR! By analyzing the history of the files in this pull request, we identified @JohnSundell to be a potential reviewer.

@@ -32,6 +32,8 @@ class DataSourceTests: XCTestCase {
let itemCell3 = component.componentDataSource!.tableView(tableView, cellForRowAt: IndexPath(row: 2, section: 0))
XCTAssertNotNil(itemCell3)

component.tableView?.reloadData()
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

🤔

Copy link
Owner Author

Choose a reason for hiding this comment

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

If we don't do this, it will throw an exception that we are trying to deque the same index path twice with a different identifier.

@codecov-io
Copy link

codecov-io commented Jun 9, 2017

Codecov Report

Merging #647 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #647      +/-   ##
==========================================
- Coverage   87.82%   87.81%   -0.01%     
==========================================
  Files         126      126              
  Lines       10312    10310       -2     
==========================================
- Hits         9056     9054       -2     
  Misses       1256     1256
Flag Coverage Δ
#ios 89.53% <100%> (-0.01%) ⬇️
#osx 90.56% <100%> (-0.01%) ⬇️
#tvos 81.57% <60%> (-0.01%) ⬇️
Impacted Files Coverage Δ
SpotsTests/Shared/TestStateCache.swift 83.92% <100%> (ø) ⬆️
SpotsTests/iOS/TestDataSource.swift 82.35% <100%> (+0.35%) ⬆️
Sources/Shared/Structs/StateCache.swift 92% <100%> (-1.11%) ⬇️
SpotsTests/iOS/ComponentiOSTests.swift 98.03% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42291ad...e07ee46. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Jun 9, 2017

Codecov Report

Merging #647 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #647      +/-   ##
==========================================
- Coverage   87.82%   87.81%   -0.01%     
==========================================
  Files         126      126              
  Lines       10312    10310       -2     
==========================================
- Hits         9056     9054       -2     
  Misses       1256     1256
Flag Coverage Δ
#ios 89.53% <100%> (-0.01%) ⬇️
#osx 90.56% <100%> (-0.01%) ⬇️
#tvos 81.57% <60%> (-0.01%) ⬇️
Impacted Files Coverage Δ
SpotsTests/Shared/TestStateCache.swift 83.92% <100%> (ø) ⬆️
SpotsTests/iOS/ComponentiOSTests.swift 98.03% <100%> (ø)
SpotsTests/iOS/TestDataSource.swift 82.35% <100%> (+0.35%) ⬆️
Sources/Shared/Structs/StateCache.swift 92% <100%> (-1.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42291ad...e07ee46. Read the comment docs.

completion?()
}
try? cache.clear()
completion?()
}

/// The StateCache file name
///
/// - returns: An md5 representation of the StateCache's file name, computed from the StateCache key
func fileName() -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need this function? @zenangst

Copy link
Owner Author

Choose a reason for hiding this comment

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

We use it for the logging feature if you use live reloading :)

@@ -1,6 +1,6 @@
import Foundation
import Cache
import CryptoSwift
import SwiftHash
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we remove fileName then we don't need this import anymore.

}

/// Load JSON from cache
///
/// - returns: A Swift dictionary
public func load() -> [String : Any] {
return SyncCache(cache).object(key)?.object as? [String : Any] ?? [:]
guard let cachedDictionary = cache.object(forKey: key)?.object as? [String: Any] else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about guard .dictionary(let cachedDictionary) = cache.object(forKey: key)

Copy link
Owner Author

Choose a reason for hiding this comment

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

@vadymmarkov how do you mean exactly? 🤔

Copy link
Owner Author

Choose a reason for hiding this comment

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

Where does .dictionary come from?

Copy link
Collaborator

Choose a reason for hiding this comment

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

JSON.dictionary from Cache

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant guard case .dictionary(let cachedDictionary) = cache.object(forKey: key)

@zenangst zenangst merged commit b7c32e9 into master Jun 9, 2017
@zenangst zenangst deleted the migrate/cache-version branch June 9, 2017 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants