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: structured-swift3/4 crash when a key ends with a . #528

Closed
AlexIzh opened this issue Oct 1, 2018 · 10 comments
Closed

Strings: structured-swift3/4 crash when a key ends with a . #528

AlexIzh opened this issue Oct 1, 2018 · 10 comments
Milestone

Comments

@AlexIzh
Copy link

AlexIzh commented Oct 1, 2018

swiftgen --version
SwiftGen v6.0.0 (Stencil v0.13.1, StencilSwiftKit v2.7.0, SwiftGenKit v6.0.0)

Example:

alex$ swiftgen strings Localizable.strings --templateName structured-swift3
Illegal instruction: 4

alex$ swiftgen strings Localizable.strings --templateName structured-swift4
Illegal instruction: 4

at the same time flat-swift3 and flat-swift4 work like a charm

@djbe
Copy link
Member

djbe commented Oct 1, 2018

Can you share your strings file?

@AlexIzh
Copy link
Author

AlexIzh commented Oct 1, 2018

Ok, I've found the problem. It was in this line

"*HINWEIS:* Organisationen besitzen keine Leserechte für die von Nachbarn erstellten Inhalte. Sie sehen nur Reaktionen auf Beiträge, die von der Organisation selbst verfasst wurden." = "*HINWEIS:* Organisationen besitzen keine Leserechte für die von Nachbarn erstellten Inhalte. Sie sehen nur Reaktionen auf Beiträge, die von der Organisation selbst verfasst wurden.";

Before it was not generated at all. But some strings like this were generated like

public static let _HINWEIS___Erweiterungen_Können_Nach_Dem_Speichern_Nicht_Rückgängig_Gemacht_Werden = L10nBeta.tr("Localizable", "*HINWEIS:* Erweiterungen können nach dem Speichern nicht rückgängig gemacht werden")

but now we've got the problem.
I'll try to change my local template to fix the issue.

Thanks!

P.S. problem that in the project we have a lot of this type of strings, because we are on migrating from it to swiftgen. And now some strings are used with old way, but some with new swiftgen way.
P.S.S. Before we used custom template, but it was broken after update to 6.0 version.

@AliSoftware
Copy link
Collaborator

AliSoftware commented Oct 1, 2018

Thanks for the feedback, that will help us zeroing on the source of the bug.

So it's probably indeed because your key uses some non-ASCII characters that make Stencil segfault.

Of course, your Localizable.strings would be better using more standardised keys, rather than use the localised string as the key. But as you said, that's because you're in the process of migrating, so I guess you are already moving towards this, thus can't migrate them all easily in one pass. And it's still not normal that it crashes like that in our end.

Might be that we did some changes in the swiftIdentifier filter — that is supposed to escape characters that are invalid in a swift identifier like the name of a let constant — that broke it. But if that was just that, you'd have swiftgen generate correctly without crashing, and then the compiler complaining (or crashing) only during compilation of the generated code.

So imho since it's crashing during the generation itself, it's more probable that it's a segfault in the code of the Stencil library itself.

@AliSoftware
Copy link
Collaborator

Oh wait, I found the reason. Nothing to do with non-ASCII characters.

That's because the key ends with a .. Just remove the . at the end of the key (you can keep it in the value/translation) and this doesn't segfault anymore.

Which actually makes kinda sense, because as you said in your initial report, that crash only occurs with structured-swift4 templates, not flat-swift4, and one key different is that structured-swift4 splits the keys using the . as separator… So I guess we missed checking if one of those parts is empty after the split

@AlexIzh
Copy link
Author

AlexIzh commented Oct 1, 2018

Yeah, makes sense!
Many thanks!

@AlexIzh AlexIzh closed this as completed Oct 1, 2018
@djbe djbe changed the title structured-swift3 and structured-swift-4 don't work with swiftgen 6.0.0 Strings: structured-swift3/4 crash when a key ends with a . Oct 1, 2018
@djbe
Copy link
Member

djbe commented Oct 1, 2018

Let's keep the issue around until we can get it properly tested and fixed

@djbe djbe reopened this Oct 1, 2018
@djbe djbe added this to the SwiftGen 6.0.1 milestone Oct 1, 2018
@AliSoftware
Copy link
Collaborator

@AlexIzh side note: since you seem to be migrating from Localizable.strings having the translation as the key to Localizable.strings having structured keys (as you were trying to use the structured-swift4 template, so I'm guessing you're interested in the feature of having structured keys with . indicating the different "levels"), I'd suggest that you use two separate .strings file during your migration, if that helps:

  • Keep the strings that are not migrated to have renamed keys yet inside Localizable.strings, so that you won't have to change anything yet for them to work
  • Move the strings for which you rename the keys into a dedicated Structured.strings (or any other name) strings file
  • Make SwiftGen parse only the Structured.strings file, so that it generates L10.foo.bar.baz keys for those strings for which you'd have migrated and renamed their keys (SwiftGen will automatically know in which .strings file to look, nothing special to add, that's the beauty of it)

This way your constants generated by SwiftGen won't be "polluted" by the old keys you haven't had time to rename yet.

And then you can iterate over your code to move some keys batch by batch to the Structured.strings file while renaming their keys, which would add the constants to L10n automatically as you do so.

(And once you've migrated all the keys, you could always remove the empty Localized.strings and rename Structured.strings to Localized.strings if you want)

That's just a suggestion, maybe it won't fit your migration process idk, but that's what we used on a legacy project when we had to do a similar migration 😉

PS: You might also be interested in adding SwiftLint rules to your project to show you warnings on your code about keys left to migrate 😉

@AlexIzh
Copy link
Author

AlexIzh commented Oct 1, 2018

Thanks, that sounds much better than what we have right now. I'll figure it out, don't know how it will work with server-side translation tools, like phraseapp. But anyway, this solution much more cleaner, thanks for that!

@AliSoftware
Copy link
Collaborator

AliSoftware commented Oct 1, 2018

FWIW, we use POEditor (+ the poesie tool), which is a competitor of PhraseApp but similar concept, and indeed that was an issue at first. But with POEditor you can add tags to translations, and then ask to export only keys with that tag, or keys without that tag, so that's what we used in the end.

So if PhraseApp offers a similar feature, you could imagine tag translations that have been updated with a nicer key, and ask phraseapp pull to export only those keys with the tags in Structured.strings and those with no tags still in Localizable.strings? (Not sure if PhraseApp has such tagging and/or if phraseapp pull offers such a filtering option though, didn't check)

@AliSoftware
Copy link
Collaborator

@AlexIzh FYI, SwiftGen 6.0.1 has just been released, and includes the #531 fix for this bug 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants