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

Convert and copy all translations at build time #14

Merged
merged 6 commits into from
Mar 30, 2017
Merged

Conversation

1ec5
Copy link
Member

@1ec5 1ec5 commented Mar 23, 2017

This PR improves the process of producing the project’s plists, preventing us from having to push PRs for each change to translations in the main osrm-text-instructions repository. Also, every language included by osrm-text-instructions is also included in this port.

A script that runs as part of the build (for Carthage users) or as a preflight step (for CocoaPods users) updates the submodule, converts the translation JSON files into plist format, then copies the plists into the built product bundle. The checked-in instructions plists have been deleted, since they can be regenerated on each build.

Fixes #10.

/cc @bsudekum @frederoni

@1ec5 1ec5 self-assigned this Mar 23, 2017
@1ec5 1ec5 requested a review from freenerd March 23, 2017 19:54
@1ec5
Copy link
Member Author

1ec5 commented Mar 23, 2017

Before merging this PR, I’d like to merge #13, which fixes the CocoaPods podspec.

The conversion script now updates the submodule and copies the plists into the built product bundle. Every available language is copied. Deleted the checked-in instructions plists in favor of a Run Script build phase that runs this script. Also run the script as a preflight command in the podspec for CocoaPods users. Deleted the separate scheme that ran the script, now that everything is automatic.
@1ec5
Copy link
Member Author

1ec5 commented Mar 24, 2017

This PR also pulls in Project-OSRM/osrm-text-instructions@b4c5187. All in all, the following languages have been added:

  • Dutch
  • French
  • German
  • Russian
  • Swedish
  • Vietnamese

@1ec5
Copy link
Member Author

1ec5 commented Mar 24, 2017

The CocoaPods podspec linter is erroring out because the resources path no longer matches any checked-in files:

    - ERROR | file patterns: The `resources` pattern did not match any file.

Some of the changes to the conversion script in 7f2eadc will need to be conditionalized: in Xcode (for Carthage), the files would be written into the build product; when run standalone (for CocoaPods), the files would be written to an easily accessed location, such as under OSRMTextInstructions/*.lproj/.

When the conversion script is run standalone, as CocoaPods does, put the plists under the source directory, since the build products directory is unknown.
@1ec5
Copy link
Member Author

1ec5 commented Mar 24, 2017

ef3fdec fixes the CocoaPods installation error.

json2plist.sh Outdated
LANGUAGE=${LANGUAGE%.json}
fi

if [ "$LANGUAGE" != "skip" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

when will language ever be skip?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. That can be removed.

@freenerd
Copy link
Member

I'm good with this approach. The only downside I see is that the git submodule checked into .git as part of osrm-text-instructions.swift will over time very much fall behind. This may be confusing for users, given that they see a submodule that is not actually used. Instead, it might make sense to the submodule only in json2plist.sh and remove it after again? This way the working git directory stays clean.

@freenerd
Copy link
Member

On second thought I think this PR is a bad idea.

When we do structural changes on the language files in osrm-text-instructions we will release a new version there. osrm-text-instructions.swift has no concept of versions but pulls in master. This means that it would pull in these new files and would break (or even worse: have undetected side-effects). This happens without any change in osrm-text-instructions.swift and is therefore a surprising side-effect. Instead, there should be a PR in osrm-text-instructions.swift with the new version (and a passing test suite).

I understand that the premise of this PR is to speed-up the process of changes in Transifex being available in osrm-text-instructions.swift, but the risk of the approach here seems greater than that benefit.

@1ec5
Copy link
Member Author

1ec5 commented Mar 27, 2017

The risk associated with structural changes upstream should be no different than it is now on master. Note that git submodule update doesn't change the commit that the submodule points to; it only ensures that the local clone points to the correct commit. (To float on master, the script would have to cd into the submodule directory and git checkout master.)

@1ec5
Copy link
Member Author

1ec5 commented Mar 29, 2017

@freenerd, as a case in point, if you run the script in this PR, you’ll wind up with Project-OSRM/osrm-text-instructions@b4c5187, not the latest master (currently Project-OSRM/osrm-text-instructions@6a57291).

Also, to clarify, the point of this PR is to avoid having to PR changes to checked-in plists. A PR is still required to get new translations, but such PRs will only require changes to the submodule pin, as was the case with acea632.

@freenerd
Copy link
Member

@1ec5 thanks for the clarification, this makes sense.

@1ec5
Copy link
Member Author

1ec5 commented Mar 30, 2017

Yay! By the way, the build failures are because .travis.yml won’t exist until #17 lands, but Travis (unlike Bitrise) insists that the file has to exist on every branch simultaneously. Only a temporary inconvenience.

@1ec5 1ec5 merged commit cacd9b3 into master Mar 30, 2017
@1ec5 1ec5 deleted the 1ec5-schemes branch March 30, 2017 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants