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.
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
Merge 21.0 (21.0.0) code freeze into
trunk
#19472Merge 21.0 (21.0.0) code freeze into
trunk
#19472Changes from 10 commits
c6328b5
ff147e8
cde05ad
5ac3202
b4dfb74
3fc0ac0
d751a7a
6b3286d
65bcdd4
6e9225f
47c1500
7e7210d
daec387
cd7b131
ede3b8e
611dd09
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Nice catch!
Not sure how that was missed in the original PR 😓, did
gentrings
warned you about it during code freeze, telling you that the parameters have to be static values? If so, maybe we should rungenstrings
on CI on each PR (even if we don't commit the generatedLocalizable.strings
on those PR CI runs, but just to gather potential warnings generated by it)? 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, phew. The error was:
Yes! I thought about that too but didn't flesh out how it could work. I'd like a dedicated step, so the failure message is clear, but I worry if it's a waste to spin up a macOS box etc. "just" for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. Maybe we could integrate that in an existing step (e.g. the one already doing
swiftlint
)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even that conditional about
differenceSign
is making assumptions about the fact that those two variants would always be translated the same with just the added+
in all languages — while depending on how different locales decide to display those values according to locale rules and practices, that might not be true (e.g. either they might put the+
sign elsewhere like after instead of before in some locales, or they might have some other character for those cases, just like Arabic has a different Unicode character for the % sign, aka U+066A, or something else…)So personally I'd even suggest to provide 4 different strings and not just 2, to cover all cases while allowing polyglots to still provide all variants matching their locale rules.
(See also: pbMoDN-3tH-p2#the-tips)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, we shouldn't be assuming particular symbols or their order in different languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. On it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I was at it, I also added reverse-DNS keys to that we can avoid ambiguous translations. See #19028.
FYI @staskus, as per this commit.
Also, I noticed there is no unit test for this logic. It would be great to add some, even though they wouldn't have caught this issue in particular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mokagio
Thank you for noticing the issue and making the fixes! 🤝
Should I add additional unit tests and additional cases that @AliSoftware was mentioning as a part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this file clean up what was most likely an incorrect merge on the project file.
See 3fc0ac0.