Skip to content

Conversation

@pbodsk
Copy link
Contributor

@pbodsk pbodsk commented Jan 19, 2022

What is This?

This is a continuation of the discussion we had on Slack yesterday related to NStack and what to do when you .never want to update your translations.

The problem seemed to be that we always automatically always go straight for the persisted data first in createLocalizationObject before we attempt a fallback to the JSON file with translations in the app.

This works fine if you automatically fetch and save/persist new content from NStack but it doesn't catch new updates if you insist on .never updating your translations. That means that if you add new translations to the JSON file, they won't be detected since the old/persisted data is used...and that doesn't contain the new translations.

The Fix

So...this is part of a two step rocket. One part here and one in the NStackSDK project.

In this project we now allow outsiders (that means you NStackSDK) to say "you know what...I don't want to look up the lookupPersistedLocalizations first, lemme go straight to the JSON file".

This property has a default value of true to minimize the damage in code already using the function.

With that in place, we can now use this from the NStackSDK to actually skip looking for persisted data...but that is the topic for a future PR in that repo 😄

Furthermore

I "fixed" some of the failing tests (15 out of 33 tests were failing in the develop branch before I touched anything 😊 )

Still have 3 failing test but I'm too stupid to fix them so....knock yourself out guys 😄

@johsoe
Copy link

johsoe commented Jan 19, 2022

Why introduce a new variable instead of just checking the updateMode?

Are there scenarios I'm missing where this wouldn't be enough?

@pbodsk
Copy link
Contributor Author

pbodsk commented Jan 19, 2022

@johsoe you're not wrong 😄

The thing is that the LocalizationManager is used by the NStackSDK project (https://github.com/nstack-io/nstack-ios-sdk), and it is the NStackSDK project who has knowledge of the updateMode variable/enum.

So...in the NStackSDK project I had to make a change as well to use currentMode to determine whether to flip lookupPersistedLocalizations to true or false.

That change will be covered by another PR in the NStackSDK project but for the complete picture, here is how I call LocalizationManager in NStackSDK...hope I haven't spoiled the suspense related to the upcoming PR in NStackSDK

        let manager = LocalizationManager<DefaultLanguage, LocalizationConfig>(
             repository: repository,
             contextRepository: repository,
             localizableModel: configuration.localizationClass,
             updateMode: .manual,
             lookupPersistedLocalizations: !configuration.updateOptions.contains(.never)
         )

@pbodsk
Copy link
Contributor Author

pbodsk commented Jan 19, 2022

Oh hey...sorry, I misread. You said .updateMode sorry, I misread that as .updateOptions (which is what is in the NStackSDK).

I was considering that but .updateMode has the values

    case automatic
    case manual

And truth be told I wasn't sure if I could squeeze in another value without side effects.

I can take another swing at this. Seems we only use .manual from NStackSDK

and introduces new updateMode enum value instead
@pbodsk
Copy link
Contributor Author

pbodsk commented Jan 19, 2022

There we go, updated

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.

3 participants