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

Use string catalogues and fix non-localized strings #33

Merged
merged 16 commits into from
Nov 7, 2023

Conversation

vishnuravi
Copy link
Member

@vishnuravi vishnuravi commented Oct 31, 2023

Use string catalogues and fix non-localized strings

⚙️ Release Notes

Migrates to use of string catalogues for localization and fixes a string that is not being drawn from the localization files.

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@vishnuravi
Copy link
Member Author

@Supereg any idea why we are getting the discouraged_optional_collection error here? Those files were not touched and seemed to have passed before?

@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #33 (8051627) into main (5bbeed7) will decrease coverage by 76.41%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #33       +/-   ##
==========================================
- Coverage   76.42%   0.00%   -76.41%     
==========================================
  Files         114     114               
  Lines        3952    3964       +12     
==========================================
- Hits         3020       0     -3020     
- Misses        932    3964     +3032     
Files Coverage Δ
Sources/SpeziAccount/AccountSetup.swift 0.00% <0.00%> (-72.50%) ⬇️
...s/SpeziAccount/Mock/MockSimpleAccountService.swift 0.00% <0.00%> (ø)
...count/Views/AccountSetup/ExistingAccountView.swift 0.00% <0.00%> (-67.85%) ⬇️
...s/SpeziAccount/Views/UserIdPasswordResetView.swift 0.00% <0.00%> (-85.29%) ⬇️
.../SpeziAccount/AccountValue/Keys/AccountIdKey.swift 0.00% <0.00%> (-21.42%) ⬇️
...count/ViewModel/AccountOverviewFormViewModel.swift 0.00% <0.00%> (-93.45%) ⬇️
...SpeziAccount/Views/DataDisplay/SimpleTextRow.swift 0.00% <0.00%> (-72.22%) ⬇️
...iews/AccountOverview/AccountOverviewSections.swift 0.00% <0.00%> (-89.08%) ⬇️
Sources/SpeziAccount/AccountOverview.swift 0.00% <0.00%> (-56.36%) ⬇️
...ment/AccountServiceConfiguration+Environment.swift 0.00% <0.00%> (-100.00%) ⬇️
... and 1 more

... and 94 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@Supereg
Copy link
Member

Supereg commented Oct 31, 2023

@Supereg any idea why we are getting the discouraged_optional_collection error here? Those files were not touched and seemed to have passed before?

Not sure why they pop up now. But feel free to add some //swiftlint:disable:this discouraged_optional_collection to the lines.

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Thank you 🚀

@vishnuravi
Copy link
Member Author

@Supereg any idea why we are getting the discouraged_optional_collection error here? Those files were not touched and seemed to have passed before?

Not sure why they pop up now. But feel free to add some //swiftlint:disable:this discouraged_optional_collection to the lines.

Thanks. I added this and then I got a line length - sometimes you just can't win!!

@Supereg
Copy link
Member

Supereg commented Oct 31, 2023

@Supereg any idea why we are getting the discouraged_optional_collection error here? Those files were not touched and seemed to have passed before?

Not sure why they pop up now. But feel free to add some //swiftlint:disable:this discouraged_optional_collection to the lines.

Thanks. I added this and then I got a line length - sometimes you just can't win!!

There is also a swiftlint:disable:previous ... which might be helpful here 😅

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

@vishnuravi @Supereg Thank you for this fix!

Can we use this PR to directly resolve #22 which would have also been helpful to be done right in #32 to ensure that we don't run into the same issues again?
String catalogues should flag these issues and should automate these processes in the future 👍

@PSchmiedmayer PSchmiedmayer mentioned this pull request Oct 31, 2023
1 task
@vishnuravi vishnuravi changed the title Fix non-localized string Use string catalogues and fix non-localized strings Oct 31, 2023
Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Thanks for the great work 🚀

As we migrated to String catalogues, we have some strings being picked up from the mock or preview implementations. I annotated every localization key I could find.
Would be great if you could review the usages of those keys and check if it makes sense to use the Text(verbatim:) initializer instead, or, otherwise, add a translation for those strings.

Otherwise, the PR looks great, thanks for the effort 🎉

Sources/SpeziAccount/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Sources/SpeziAccount/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Sources/SpeziAccount/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Sources/SpeziAccount/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Sources/SpeziAccount/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Sources/SpeziAccount/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Sources/SpeziAccount/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you @vishnuravi! Happy to get the PR merged once @Supereg is happy and his comments are resolved 🚀

Package.swift Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Would be great if we can also just include the MD link check in there as well, just so we get it into the different repos as we make different smaller or bigger PRs 👍

@PSchmiedmayer
Copy link
Member

@vishnuravi Following up on our discussion today: I addressed most of the issues with the localization files and only needed to translate a few very limited elements and added comments for the rest.

Looking forward to getting the PR finished and feel free to merge it once all the conversations are resolved 👍

@Supereg
Copy link
Member

Supereg commented Nov 6, 2023

fyi, I'm currently rebasing this PR and taking care of it getting it merged if that is alright for you @vishnuravi 👍

@vishnuravi
Copy link
Member Author

fyi, I'm currently rebasing this PR and taking care of it getting it merged if that is alright for you @vishnuravi 👍

That sounds great, thank you @Supereg!

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for finishing the PR @Supereg and thanks for all the work here @vishnuravi!

@PSchmiedmayer PSchmiedmayer merged commit 80bd6be into main Nov 7, 2023
6 of 8 checks passed
@PSchmiedmayer PSchmiedmayer deleted the fix-localization branch November 7, 2023 00:11
@vishnuravi vishnuravi mentioned this pull request Nov 11, 2023
1 task
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