Deprecate old strings resources #718
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gersonnoboa
requested review from
igorkravchenko,
EgorovEI,
rasmustautsglia and
yurii-glia
| // swiftlint:disable nesting type_body_length type_name vertical_whitespace_opening_braces | ||
| internal enum Localization { | ||
| internal enum Alert { | ||
| internal enum Action { |
There was a problem hiding this comment.
Is there a way we could potentially make the indentation bigger to conform to swift standards? As this file can be generated from the command line, it's not a big issue but it would just look better
There was a problem hiding this comment.
I could take a look, probably there's a parameter.
There was a problem hiding this comment.
I investigated, apparently there's no way of doing this. In theory, I could modify the stencil so that it has four spaces. This would deviate us greatly from the official one. Wouldn't be the worst but we would have to decide if it's worth it. A benefit of being close to the original is that if they update it, we can just add our few changes easily. What do you think would be best?
yurii-glia
reviewed
Aug 14, 2023
EgorovEI
approved these changes
Aug 14, 2023
yurii-glia
approved these changes
Aug 14, 2023
igorkravchenko
approved these changes
Aug 14, 2023
gersonnoboa
force-pushed
the
MOB-2529
branch
2 times, most recently
from
4aa88d5
to
e980848
Compare
Instead of trying to reuse some old strings, deprecate some other strings, and have them coexist in the same file, I have decided to deprecate the `L10n.swift` class altogether, and create a new one called `Localization.swift`. This allows me to remove that horrible abbreviated name, and also make a clear distinction between old and new. With this, all new strings will just go to the `Localizable.strings` as always, and the `Deprecated.strings` ones shouldn't be used anymore. This was done by adding two `.stencil` files: one that handles adding the `@deprecated` line to the old file, and a new one that handles ignoring all strings that start with `android`. This is because on the new way to do strings, the Android-specific ones will start with `android.`. We could remove these from the file, but this would be manual, some could be missed, and at the moment of this PR, we haven't yet determined how we are going to sync these strings with the strings in the Glia Hub. Finally, the new `Localization.swift` file is `internal`, as opposed to `L10n.swift` which is public. This is because these strings have changed from being the main way to show strings in the widgets, to being a fallback. Now, the Glia Hub will be the main way to pass strings to the widgets, so there's no need to shoot ourselves in the foot, making this public, and then have to worry about deprecations, as we have to do with `L10n.swift`. With this, there's no need to have `L10n+BackwardsCompatibility.swift`, as the new strings and the old ones are separated entities. However, I still need to do a layer that goes between the strings and the rest of the code, for when the strings have functionality that is not present anymore (example, placeholders for values that now just have the direct value in the string). In a year, we can easily remove `L10n.swift` and the associated `.stencil` file. MOB-2529
|
!merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of trying to reuse some old strings, deprecate some other strings, and have them coexist in the same file, I have decided to deprecate the
L10n.swiftclass altogether, and create a new one calledLocalization.swift. This allows me to remove that horrible abbreviated name, and also make a clear distinction between old and new. With this, all new strings will just go to theLocalizable.stringsas always, and theDeprecated.stringsones shouldn't be used anymore.This was done by adding two
.stencilfiles: one that handles adding the@deprecatedline to the old file, and a new one that handles ignoring all strings that start withandroid. This is because on the new way to do strings, the Android-specific ones will start withandroid.. We could remove these from the file, but this would be manual, some could be missed, and at the moment of this PR, we haven't yet determined how we are going to sync these strings with the strings in the Glia Hub.Finally, the new
Localization.swiftfile isinternal, as opposed toL10n.swiftwhich is public. This is because these strings have changed from being the main way to show strings in the widgets, to being a fallback. Now, the Glia Hub will be the main way to pass strings to the widgets, so there's no need to shoot ourselves in the foot, making this public, and then have to worry about deprecations, as we have to do withL10n.swift.With this, there's no need to have
L10n+BackwardsCompatibility.swift, as the new strings and the old ones are separated entities. However, I still need to do a layer that goes between the strings and the rest of the code, for when the strings have functionality that is not present anymore (example, placeholders for values that now just have the direct value in the string).In a year, we can easily remove
L10n.swiftand the associated.stencilfile.MOB-2529