Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Multiple strings tables support #41

Merged
merged 4 commits into from
May 28, 2017
Merged

Conversation

djbe
Copy link
Member

@djbe djbe commented May 22, 2017

This partially adresses SwiftGen/SwiftGen#39, it would still need an implementation on the SwiftGen (CLI) side, and of course also one on the templates side.

Refs SwiftGen/templates#49

@AliSoftware
Copy link
Contributor

I'll try to look into #40 and #41 on thursday 👍 (4-days week-end 🎉 )

@AliSoftware
Copy link
Contributor

This might require additional Unit Tests though, especially at least one expected output containing multiple tables (to be sure we don't override entries on every new table added for example)

Also, what about if the tables have the same name, because the user misunderstood the feature and passes fr.lproj/Localizable.strings en.lproj/Localizable.strings as parameters? Or even if it's 2 strings of different frameworks and they don't realize they're gonna conflict, like MyFmk.framework/Base.lproj/Localizable.strings OtherFmk.framework/Base.lproj/Localizable.strings?

Should we make the last one override the previous ones? Merge them? Generate an {% error %}? (here's that wishful tag again 😄)

@djbe
Copy link
Member Author

djbe commented May 23, 2017

Hmmm. I think we should throw an error if the user provides multiple tables with the same name. And this error should be thrown by SwiftGenKit, not in Stencil.

If a user wants to generate code for 2 different targets, then they should run it twice.

Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

Other than the wording, LGTM


public var description: String {
switch self {
case .failureOnLoading(let path):
return "Failed to load a file at \"\(path)\""
case .invalidFormat:
return "Invalid strings file"
case .duplicateTable(let name):
return "Table \"\(name)\" already loaded, cannot add it again"
Copy link
Contributor

Choose a reason for hiding this comment

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

The real problem isn't that we cannot add it again (to be fair, we could totally merge the entries form both tables in a single context entry), but more that it wouldn't make sense, because:

  • either both tables with the same name come from the same target but for different languages so that would lead to parsing the same entries — just with different translations
  • or because they come from different targets (a framework and an app for example) and that wouldn't make sense to merge them in a single huge file while they should be split in different files belonging to their respective targets

So maybe we should find a better wording/explanation for this error? Like this?

Duplicate table \"\(name)\". Only provide all your strings files for a *single* language (typically your base language) from *one* given target (app or framework).

Or if you have a better wording don't hesitate 😉

@AliSoftware AliSoftware force-pushed the feature/strings-multiple-tables branch from 29ea509 to 61c2162 Compare May 27, 2017 18:58
@AliSoftware
Copy link
Contributor

@djbe Rebased but with conflicts so I'll let you double-check (esp the sync with SwiftGen/templates#49) before merging

@AliSoftware
Copy link
Contributor

AliSoftware commented May 27, 2017

⚠️ We might want to merge #44 first (because it modifies the code around the addEntry function while this #41 removes it) to reduce the conflicts

@djbe djbe force-pushed the feature/strings-multiple-tables branch from 61c2162 to 1634f06 Compare May 28, 2017 20:52
@djbe djbe force-pushed the feature/strings-multiple-tables branch from 1634f06 to 52dafe3 Compare May 28, 2017 21:13
@djbe djbe merged commit 91e3239 into master May 28, 2017
@djbe djbe deleted the feature/strings-multiple-tables branch May 28, 2017 22:07
@djbe djbe mentioned this pull request Aug 1, 2017
23 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants