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

Fix a problem that prevents proper support for variable keys #2

Merged
merged 2 commits into from
Feb 7, 2015

Conversation

danielmartin
Copy link

If you try to pass a variable key to the SPLocalizedString/SPLocalizedStringPlural macros, the program doesn't even compile due to a problem with C macro expansion. NSLocalizedString supports variable keys, so SPLocalizedString should too.

This pull request implements the changes required so that you can pass either a constant or a variable key to SPLocalizedString/SPLocalizedStringPlural.

I've also modified the sample project to test that the fix is working fine and no regressions are introduced.

@sergiou87
Copy link
Owner

Thanks for your work @danielmartin !!

I like the idea but I think this change doesn't get along with spgenstrings (which is part of the "package"). It doesn't crash but look at the result of running spgenstrings on SPExample.m:

Localizable.strings:

"(Context for variable string)variable" = "variable";

"(First hello world)Hello world!" = "Hello world!";

"(Followers label description##one)%d people is following you" = "Just one person is following you";

"(Followers label description##other)%d people is following you" = "%d people are following you";

"(Friends counter##one)variablePlural" = "variablePlural";

"(Friends counter##other)variablePlural" = "variablePlural";

"(Second hello world)Hello world!" = "Hello world again";

OtherTable.strings:

"(Another text from another table)Hello world!" = "Hello world from another table!";

"(Another variable text from another table)variableTable" = "variableTable";

I changed the example to work with NSLocalizedString and check how it works with genstrings from Apple and this is the result:

$ genstrings -o . SPExample.m
Bad entry in file SPExample.m (line = 41): Argument is not a literal string.
Bad entry in file SPExample.m (line = 42): Argument is not a literal string.
Bad entry in file SPExample.m (line = 46): Argument is not a literal string.
Bad entry in file SPExample.m (line = 47): Argument is not a literal string.

I don't like it failing miserably, but I don't like it just working without informing the user. Would you mind editing spgenstrings to emit a warning (with line number and file name, if possible) when it finds a key, text or table name that is not a literal string?

If you want to test your changes in spgenstrings you only have to run the project, it already has the arguments to analyze SPExample.m and update the strings files you already know 😄

…ext, or table name is a non-literal string
@sergiou87
Copy link
Owner

@danielmartin Wow! I just noticed you changed spgenstrings already, you're my hero! 😄 Is that all or does it need more changes?

@danielmartin
Copy link
Author

@sergiou87 I think that's enough. The change does warn the user if a non-literal string is found in keys, contexts, or table names (indicating file name and line number, same information as in genstrings). The diff is very local to spgenstrings's scanner.

I think there's still a problem when there are comments inside SPLocalizedString parameters (there is a TODO comment about that that comes from the first release, I think), but this is a matter of a future pull request. 😄

@sergiou87
Copy link
Owner

Yeah, the problem is it's based on parsing the code files manually, rather than using something like libclang (which actually understands Objective-C… and Swift when it's needed 😄 ). I started writing a tool in Python based on libclang but I couldn't get enough free time to finish it 😕

Anyway, thank you very much for your contribution, I'll add you to the README file too, and I hope I can get you a 🍺 some time!! 😄

sergiou87 added a commit that referenced this pull request Feb 7, 2015
Fix a problem that prevents proper support for variable keys
@sergiou87 sergiou87 merged commit cfb122b into sergiou87:master Feb 7, 2015
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.

2 participants