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

Extract i18n string into i18n/_defaults (base of translations) #4514

Merged
merged 21 commits into from
Feb 14, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Feb 10, 2017

All i8n/_default/* files generated by the scripts/build-i18n.js script.

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Feb 10, 2017

import i18nstrings from '../.build/i18n/i18n/en.json';

const FILE_HEADER = '// Copyright 2015-2017 Parity Technologies (UK) Ltd.\n// This file is part of Parity.\n\n// Parity is free software: you can redistribute it and/or modify\n// it under the terms of the GNU General Public License as published by\n// the Free Software Foundation, either version 3 of the License, or\n// (at your option) any later version.\n\n// Parity is distributed in the hope that it will be useful,\n// but WITHOUT ANY WARRANTY; without even the implied warranty of\n// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n// GNU General Public License for more details.\n\n// You should have received a copy of the GNU General Public License\n// along with Parity. If not, see <http://www.gnu.org/licenses/>.\n\n';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use template string here, would be more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually did have it that way around, will put it back. (Don't expect to edit it at all, but still more readable.)

import fs from 'fs';
import path from 'path';

import i18nstrings from '../.build/i18n/i18n/en.json';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is babel-node used only for these imports ? If it is, could use require I think...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const/let is supported, so only imports. I would prefer keeping the import just for code-base consistency. (Although there is a slight compiler indirection involved here)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really mind but it adds quite some work for not so much. All the other scripts are using standard Node syntax

@ngotchac
Copy link
Contributor

I would have something like that :

const fs = require('fs');
const path = require('path');

const i18nstrings = require('../.build/i18n/i18n/en.json');

const FILE_HEADER = `// Copyright 2015-2017 Parity Technologies (UK) Ltd.
// This file is part of Parity.

// Parity is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// Parity is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Parity.  If not, see <http://www.gnu.org/licenses/>.\n\n`;
const I18NPATH = path.join(__dirname, '../src/i18n/_default');

main();

function getSections () {
  const sections = {};

  // create an object map of the actual inputs
  Object.keys(i18nstrings).forEach((fullKey) => {
    const defaultMessage = i18nstrings[fullKey].defaultMessage;
    const keys = fullKey.split('.');
    let outputs = sections;

    keys.forEach((key, index) => {
      if (index === keys.length - 1) {
        outputs[key] = defaultMessage;
      } else {
        if (!outputs[key]) {
          outputs[key] = {};
        }

        outputs = outputs[key];
      }
    });
  });

  return sections;
}

function main () {
  console.log('\n-------------------------------------------------------');
  console.log('------------------ START: i18n exports ----------------');
  console.log('-------------------------------------------------------');

  const sections = getSections();

  // create the index.js file
  const sectionKeys = Object.keys(sections).sort();
  const i18Exports = sectionKeys
    .map((key) => {
      return `export ${key} from './${key}.json';`;
    })
    .join('\n');

  const indexPath = path.join(I18NPATH, 'index.js');

  console.log('  --> writing exports to', indexPath);
  fs.writeFileSync(indexPath, `${FILE_HEADER}${i18Exports}\n`, 'utf8');

  console.log('\n  --> writing', sectionKeys.length, 'exported message files in', I18NPATH);
  // create the individual section files
  sectionKeys.forEach((key) => {
    const sectionText = JSON.stringify(sections[key], null, 2);

    fs.writeFileSync(path.join(I18NPATH, `${key}.json`), `${sectionText}`, 'utf8');
  });

  console.log('\n------------------------------------------------------');
  console.log('------------------ DONE: i18n exports ----------------');
  console.log('------------------------------------------------------\n');
}

@ngotchac
Copy link
Contributor

This exports all strings as JSON, and the index.js file import the JSON files. JSON import is supported in standard Node as well as in Webpack 2

@ngotchac ngotchac added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 10, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Feb 10, 2017

As discussed on chat -

Just want to make the outputs the same as what people are to produce - in-place editing. Currently (not cast in stone, obviously), we import exactly the same formats.

It could be -

  • This format
  • JSON
  • .properties files
  • a completely flat list
  • { "some.key.goes.here": { "defaultMessage": "some message" } }
  • ...

Actually, this format is better than json for editing, however probably not the best overall. So +1 for ease, but not perfect. So open to suggestions to make editing as easy as possible. Current format is better than json (which is the standard), but def. not the best.

@jacogr
Copy link
Contributor Author

jacogr commented Feb 10, 2017

Strangely enough, your main() aligns nicely with what I have here after some cleanups - will push shortly. (I split into some extra functions, you have some extra logging - which is good.)

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Feb 10, 2017
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 11, 2017
@h3ll0fr13nd
Copy link
Contributor

@jacogr in the generated _default/settings.js many settings.xx.xx.labels are now missing, while they were there before.

@ngotchac
Copy link
Contributor

Not sure why JSON is less prone to editing than a JS Object. The script would be safer IMO (use a simple JSON.stringify instead of manually creating the whole JS exported Object => what if a field includes a ``` character ?). Plus everybody knows JSON, it's easy to read, etc..

@jacogr
Copy link
Contributor Author

jacogr commented Feb 13, 2017

JSON vs JS - No lint checking, no duplicate key checking, no option for commenting.

My preference would actually be for something where standard tools can be used, so preferably neither. (However the current is def better than standard JSON)

@jacogr
Copy link
Contributor Author

jacogr commented Feb 13, 2017

@h3ll0fr13nd Good catch. It only pulls in the strings that are actually in the source, not the special cases (where settings in one since it gets duplicated in a number of places). Will address and pull them in alongside.


// export a section as a flatenned string (non-JSON, rather JS export)
function createExportString (section, indent) {
if (Object.prototype.toString.call(section) === '[object String]') {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (typeof section === 'string')

@ngotchac
Copy link
Contributor

ngotchac commented Feb 13, 2017

Well, lint checking won't add more than the key duplicate check. For the comments, the file is generated automatically so I don't really see where comments would be added ?
And the key duplication can be checked with json I'm sure

@jacogr
Copy link
Contributor Author

jacogr commented Feb 13, 2017

Ahh, yes, these files are generated automatically. However, they are the base of what translators work on and give back.

These files are not means for reading by machines, taking them, reading, translating - the result of those will be e.g. this PR by our first external translation - https://github.com/ethcore/parity/pull/4484/files#diff-b4da45a3972c2df6a228f6eed0856c04R17)

^^^ That is what we need to make easy, JSON is not it :)

@derhuerst
Copy link
Contributor

derhuerst commented Feb 13, 2017

I'm torn between both arguments. JSON is certainly easier to edit for non-JS contributors. Also it is closer to other static i18n formats. JS has the advantage of allowing e.g. comments. Formats like json5 unfortunately require further tooling.

@ngotchac
Copy link
Contributor

@jacogr So do you think that JS Objects are more human-readable than JSON ? Not sure why that would be. Plus react-intl plugins are extracting default messages into JSON files from what I understood. So it would just be more consistent with what's existent.

For comments, we could add a description to the FormattedMessage component that we use: https://github.com/yahoo/babel-plugin-react-intl#options

@jacogr
Copy link
Contributor Author

jacogr commented Feb 13, 2017

100%, much more readable, no question, especially with nesting instead of chained keys.

(And as I stated before, I would actually prefer something else - but still need some investigation here to see what standard tools properly support. In this case it should be for use by translation tools, not humans. Most translators won't have techy chops.)

The JS as it stands will give a start to the volunteers which has technical know-how, e.g. NL & ES. (Not the end-game)

@derhuerst
Copy link
Contributor

Plus react-intl plugins are extracting default messages into JSON files from what I understood. So it would just be more consistent with what's existent.

Missed this before. This is a very strong argument for my to stick with plain JSON.

@ngotchac
Copy link
Contributor

Just to be sure, what I am suggesting is having this :

{
  "ui": {
    "account": { ... }
  }
}

instead of this :

{
  ui: {
    account: { ... }
  }
}

@jacogr
Copy link
Contributor Author

jacogr commented Feb 13, 2017

That doesn't match what is actually exported by the build plugins :) We still go through and mangle it into submission to get that format.

Your example is exactly what I did have originally, however much easier to work with that type of structure in JS as opposed to JSON, just because of

  • the tool help (highlighting, things look different)
  • linting, e.g. had issues in JSON keys which was not apparent (JS conversion highlighted them)
  • as well as ability to annotate (comments)

@jacogr
Copy link
Contributor Author

jacogr commented Feb 13, 2017

@h3ll0fr13nd It now pulls in the missing non-standard location strings as well. It should match up with what you have been working with thus far.

@@ -46,8 +48,17 @@ const SRCPATH = path.join(__dirname, '../.build/i18n/i18n/en.json');
outputIndex(sectionNames);
})();

// helper to merge in section into defaults
function deepMerge (object, source) {
Copy link
Contributor

Choose a reason for hiding this comment

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

_.defaultsDeep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooooh! Cool. Thanks.

Copy link
Contributor

@ngotchac ngotchac left a comment

Choose a reason for hiding this comment

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

Okay, that works for me ! We might now need a script that would ease translation, like copying the files to the different languages, having warning for deprecated keys (or automatically removing them if we feel confident), have some sort of status on translation...

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 14, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Feb 14, 2017

+1 I was thinking about that as well.

(Also a reason why I believe the next step is really standard tool support, just need a gap to start looking - don't want to write too much ourselves that are already handled elsewhere. However, for the people who will be editing/copying the defaults, it needs to be easy, so checking will be in order.)

@jacogr jacogr merged commit 71c0cc8 into master Feb 14, 2017
@jacogr jacogr deleted the jg-extract-strings branch February 14, 2017 12:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants