Refactor cache policies #20

stelabouras · 2021-02-16T14:47:28Z

Refactors cache policies based on the latest discussion between
@dbendilas and @Petrakeas.

The empty translations ("") are now added in the cache (depending on
the policy) and they are not filtered out.

A helpful cheat-sheet table has been added in the documentation of the
TXCacheOverridePolicy and the policy documentation has been also
updated.

The cache now defaults to the .overrideUsingTranslatedOnly instead of
the .overrideAll policy, if no policy is specified by the developer.

If a cache entry is found to contain an empty string during rendering,
the logic falls back to the missing policy.

The documentation of the TXDiskCacheProvider has been updated to
declare that it initializes the disk cache provider synchronously.

stelabouras · 2021-02-16T14:49:43Z

TODO:

More unit tests on all of the policy cases
Version number ++

dbendilas

Looks great. I've added a few comments

dbendilas · 2021-02-17T05:50:31Z

Sources/Transifex/Cache.swift

+///```
+/// | Key | Cache | New  | All  | Using Translated Only | Untranslated Only |
+/// |-----|-------|------|------|-----------------------|-------------------|
+/// | 1   | 1     | None | None | 1                     | 1                 |
+/// | 2   | 1     | 2    | 2    | 2                     | 1                 |
+/// | 3   | 1     | ""   | ""   | 1                     | 1                 |
+/// | 4   | ""    | None | None | ""                    | ""                |
+/// | 5   | ""    | 2    | 2    | 2                     | 2                 |
+/// | 6   | None  | 2    | 2    | 2                     | 2                 |
+///```


I would change this to:

| Key || Cache | New || Overide All | Using Translated Only | Untranslated Only | |-----||-------|------||--------------|-----------------------|-------------------| | a || "a" | - || - | "a" | "a" | | b || "b" | "B" || "B" | "B" | "b" | | c || "c" | "" || "" | "c" | "c" | | d || "" | - || - | "" | "" | | e || "" | "E" || "E" | "E" | "E" | | f || - | "F" || "F" | "F" | "F" |

dbendilas · 2021-02-17T06:30:29Z

Sources/Transifex/Cache.swift

+/// | 4   | ""    | None | None | ""                    | ""                |
+/// | 5   | ""    | 2    | 2    | 2                     | 2                 |
+/// | 6   | None  | 2    | 2    | 2                     | 2                 |
+///```
 @objc
 public enum TXCacheOverridePolicy : Int {
    /// All of the cache entries are replaced with the new translations.


I think that the following would be more clear, what do you think?:

/// Discards the existing cache entries completely and populates the cache with the new entries, even if they contain empty translations.

Yup that's better, I replaced it.

dbendilas · 2021-02-17T06:30:55Z

Sources/Transifex/Cache.swift

+    /// New translations update the cache (updating existing entries or adding new ones) and any cached
+    /// translations that aren't updated are left as-is.
+    /// If a cached translation exists and it's not an empty string and the value for the new translation is an
+    /// empty string, then the cached translation is left as -is.


How about simplifying it:

Updates the existing cache with the new entries that have a non-empty translation, ignoring the rest.

dbendilas · 2021-02-18T12:03:54Z

Sources/Transifex/Cache.swift

+    /// Only the translations that don't exist in the cache or cached translations that contain empty strings
+    /// are updated / added, otherwise they are left as-is.
+    /// If a cached translation exists and it's not an empty string and the value for the new translation is an
+    /// empty string, then the cached translation is left as -is.
    case overrideUntranslatedOnly


As discussed offline, let's remove overrideUntranslatedOnly completely, as it doesn't seem to cover a plausible customer flow.

dbendilas · 2021-02-18T12:04:59Z

Tests/TransifexTests/TransifexTests.swift

@@ -330,7 +330,7 @@ final class TransifexTests: XCTestCase {

        XCTAssertNil(cache.get(key: "key1", localeCode: "en"))
        XCTAssertNotNil(cache.get(key: "key2", localeCode: "en"))
-        XCTAssertNil(cache.get(key: "key3", localeCode: "en"))
+        XCTAssertNotNil(cache.get(key: "key3", localeCode: "en"))


I think we should update all tests to include all cases described in the nice table you created.

I have pushed a commit that replaces the existing policy unit tests with two tests that use the provided table.

dbendilas · 2021-02-18T12:05:59Z

Sources/Transifex/Cache.swift

    ///   - groupIdentifier: The group identifier of the app, if the app makes use of the app groups
    /// entitlement. Defaults to nil.
    @objc
-    public init(overridePolicy: TXCacheOverridePolicy = .overrideAll,
+    public init(overridePolicy: TXCacheOverridePolicy = .overrideUsingTranslatedOnly,


As discussed offline, let's use .overrideAll as the default policy.

Switched back to .overrideAll policy here.

stelabouras · 2021-02-22T10:04:01Z

@dbendilas I have just pushed a new commit that covers the points raised during review. I can leave it as WIP until cache implementation has been also finalized in the Android version.

dbendilas

Looks great!

stelabouras · 2021-03-17T09:52:28Z

@dbendilas I just pushed a final commit replacing the 'override' term with 'update' where applicable, to reflect the change of the enum. If everything looks OK from your side, let's approve and merge this PR.

dbendilas

Looks great! Please squash and I'll approve

@dbendilas

Refactors cache policies based on the latest discussion between @dbendilas and @Petrakeas. The empty translations (`""`) are now added in the cache (depending on the policy) and they are not filtered out. A helpful cheat-sheet table has been added in the documentation of the `TXCacheOverridePolicy` and the policy documentation has been also updated. The cache now defaults to the `.overrideUsingTranslatedOnly` instead of the `.overrideAll` policy, if no policy is specified by the developer. If a cache entry is found to contain an empty string during rendering, the logic falls back to the missing policy. The documentation of the `TXDiskCacheProvider` has been updated to declare that it initializes the disk cache provider synchronously. * Improves policy unit tests by using the table provided in the documentation to initialize a memory cache and a set of new translations. * Adds a unit test that tests whether the read only cache (`TXReadOnlyCacheDecorator`) is immutable. * Refactors `TXStandardCache` to use a static method that constructs and returns the `TXCache` object instead of using the decorator pattern. Updates the default standard cache usage in the `NativeCore` class and the README instructions. * Renames override policy enum to update policy and updates the values to match the ones used by the Java SDK. * Replaces occurrences of the 'override' term (relating to the override policy) with the 'update' term, to match the rename of the override policy enum. * Updates CHANGELOG, bumps version

stelabouras · 2021-03-19T09:26:33Z

Looks great! Please squash and I'll approve

Done!

dbendilas reviewed Feb 18, 2021

View reviewed changes

stelabouras force-pushed the stelios/fix/cache-policy branch from 40943d3 to 3e465f2 Compare February 24, 2021 12:57

stelabouras changed the base branch from master to devel February 24, 2021 12:59

stelabouras force-pushed the stelios/fix/cache-policy branch 2 times, most recently from 5fb3186 to 1f326e0 Compare February 24, 2021 13:04

dbendilas force-pushed the devel branch from f2a4e52 to adb327d Compare February 24, 2021 13:13

stelabouras force-pushed the stelios/fix/cache-policy branch from 1f326e0 to 9f9d5e8 Compare March 11, 2021 14:58

dbendilas previously approved these changes Mar 17, 2021

View reviewed changes

stelabouras dismissed dbendilas’s stale review via eb561e2 March 17, 2021 09:51

dbendilas reviewed Mar 19, 2021

View reviewed changes

stelabouras force-pushed the stelios/fix/cache-policy branch from eb561e2 to a3e162a Compare March 19, 2021 09:26

dbendilas changed the title ~~WIP: Refactor cache policies~~ Refactor cache policies Mar 19, 2021

dbendilas approved these changes Mar 19, 2021

View reviewed changes

dbendilas merged commit db39894 into devel Mar 19, 2021

dbendilas deleted the stelios/fix/cache-policy branch March 19, 2021 10:27

wyngarde mentioned this pull request Mar 19, 2021

Release: 0.1.1 #24

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor cache policies #20

Refactor cache policies #20

stelabouras commented Feb 16, 2021

stelabouras commented Feb 16, 2021

dbendilas left a comment

dbendilas Feb 17, 2021

stelabouras Feb 22, 2021

dbendilas Feb 17, 2021

stelabouras Feb 22, 2021

dbendilas Feb 17, 2021

stelabouras Feb 22, 2021

dbendilas Feb 18, 2021

stelabouras Feb 22, 2021

dbendilas Feb 18, 2021

stelabouras Feb 22, 2021

dbendilas Feb 18, 2021

stelabouras Feb 22, 2021

stelabouras commented Feb 22, 2021

dbendilas left a comment

stelabouras commented Mar 17, 2021

dbendilas left a comment

stelabouras commented Mar 19, 2021

Refactor cache policies #20

Refactor cache policies #20

Conversation

stelabouras commented Feb 16, 2021

stelabouras commented Feb 16, 2021

dbendilas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stelabouras commented Feb 22, 2021

dbendilas left a comment

Choose a reason for hiding this comment

stelabouras commented Mar 17, 2021

dbendilas left a comment

Choose a reason for hiding this comment

stelabouras commented Mar 19, 2021