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

Strings: reverted to case-sensitive sorting for keys #890

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

grigorye
Copy link

@grigorye grigorye commented Oct 18, 2021

This should fix #891, as well as slightly affect the performance due to no need to lowercase keys on every comparison during the sorting.

@grigorye grigorye force-pushed the feature/Case-Sensitive-Localized-Strings-Sorting branch from 1b6e7c2 to 233a4b2 Compare October 18, 2021 05:39
@grigorye grigorye changed the title Reverted to case-sensitive sorting of the localized strings. Reverted to case-sensitive sorting of the output generated for Localizable.strings. Oct 18, 2021
@grigorye grigorye marked this pull request as ready for review October 18, 2021 08:20
@djbe djbe changed the title Reverted to case-sensitive sorting of the output generated for Localizable.strings. Strings: reverted to case-sensitive sorting for keys Nov 26, 2021
@djbe djbe force-pushed the feature/Case-Sensitive-Localized-Strings-Sorting branch from 233a4b2 to cea4747 Compare July 21, 2022 00:43
@djbe
Copy link
Member

djbe commented Jul 21, 2022

Hey @grigorye I've updated the PR to use $0.key.caseInsensitiveCompare($1.key) == .orderedAscending again as discussed in #891, and also broadly applying it to the other parsers.

Mind giving it a whirl?

@djbe djbe force-pushed the feature/Case-Sensitive-Localized-Strings-Sorting branch 2 times, most recently from 19ca485 to cf4273e Compare July 22, 2022 14:43
@djbe djbe linked an issue Jul 22, 2022 that may be closed by this pull request
@djbe djbe force-pushed the feature/Case-Sensitive-Localized-Strings-Sorting branch from cf4273e to cb0617c Compare July 22, 2022 15:55
@djbe djbe mentioned this pull request Jul 22, 2022
@djbe djbe force-pushed the feature/Case-Sensitive-Localized-Strings-Sorting branch from cb0617c to 026aa23 Compare July 26, 2022 22:14
@jasdev

This comment was marked as off-topic.

@djbe

This comment was marked as off-topic.

@jasdev

This comment was marked as off-topic.

@djbe
Copy link
Member

djbe commented Aug 28, 2022

Yeah this is a very well known issue with git, encountered it myself quite a few times. The thing with those renames is that they have to happen with git mv (see here). And even then, I think other people working on the same repo with existing checkouts, it'll still be the old case 🤷

Anyway, this has little to do with SwiftGen 😅 @grigorye's original issue was with 2 keys with the same value (differing only in case). Honestly not a realistic situation, curious if anyone has a good test case for this. And if so, please try out these changes.

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.

Strings: unstable output for keys that differ just by case
3 participants