Skip to content

Add globalization#118

Merged
superkhau merged 1 commit intomasterfrom
globalization
Jul 27, 2016
Merged

Add globalization#118
superkhau merged 1 commit intomasterfrom
globalization

Conversation

@superkhau
Copy link
Contributor

@superkhau superkhau commented Jul 21, 2016

@superkhau superkhau force-pushed the globalization branch 2 times, most recently from e6eb1ea to 69a703f Compare July 21, 2016 22:18
@0candy 0candy mentioned this pull request Jul 21, 2016
Closed
63 tasks
@superkhau
Copy link
Contributor Author

superkhau commented Jul 22, 2016

@slnode test please

0candy
0candy reviewed Jul 22, 2016
View reviewed changes
lib/providers/apns.js Outdated

connection.on('transmissionError', function(code, notification, recipient) {
var err = new Error('Cannot send APNS notification: ' + code);
var err = new Error(g.f('Cannot send APNS notification: %s', code));
Copy link
Contributor

@0candy 0candy Jul 22, 2016

Choose a reason for hiding this comment

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

We don't need translation for APNS put {{}} around it.
{{APNS}}

@0candy 0candy assigned superkhau and unassigned 0candy Jul 22, 2016
@superkhau
Copy link
Contributor Author

superkhau commented Jul 23, 2016 •
edited
Loading

@0candy Updated, review again pls.

@superkhau superkhau assigned 0candy and unassigned superkhau Jul 23, 2016
0candy
0candy reviewed Jul 25, 2016
View reviewed changes
lib/providers/apns.js

connection.on('transmissionError', function(code, notification, recipient) {
var err = new Error('Cannot send APNS notification: ' + code);
var err = new Error(g.f('Cannot send {{APNS}} notification: %s', code));
Copy link
Contributor

@0candy 0candy Jul 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

You have to run slt-globalize -e again for intl/en/messages.json to get updated with the brackets change.

@superkhau
Copy link
Contributor Author

superkhau commented Jul 25, 2016

@0candy Updated, PTAL again.

@0candy
Copy link
Contributor

0candy commented Jul 25, 2016 •
edited
Loading

The change itself LGTM.
But there is a globalize error right now, all the CI tests fail.

  throw new assert.AssertionError({
        ^
AssertionError: Not supported: en
    at loadGlobalize (/mnt/jenkins/workspace/loopback-component-push/cdcdef7b/node_modules/strong-globalize/lib/globalize.js:354:3)
    at Object.setDefaultLanguage (/mnt/jenkins/workspace/loopback-component-push/cdcdef7b/node_modules/strong-globalize/lib/globalize.js:100:3)
    at new StrongGlobalize (/mnt/jenkins/workspace/loopback-component-push/cdcdef7b/node_modules/strong-globalize/index.js:27:41)
    at StrongGlobalize (/mnt/jenkins/workspace/loopback-component-push/cdcdef7b/node_modules/strong-globalize/index.js:25:12)
    at Object.<anonymous> (/mnt/jenkins/workspace/loopback-component-push/cdcdef7b/lib/providers/apns.js:11:9)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)

So please don't merge yet.

@superkhau superkhau mentioned this pull request Jul 25, 2016
Closed
Setogit
Setogit reviewed Jul 25, 2016
View reviewed changes
index.js
var loopback = require('loopback');
var PushConnector = require('./lib/push-connector');
exports = module.exports = PushConnector;
var SG = require('strong-globalize');
Copy link

@Setogit Setogit Jul 25, 2016

Choose a reason for hiding this comment

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

Why not following strong-globalize coding guideline here?
https://www.npmjs.com/package/strong-globalize#autonomous-message-loading
Please study paragraphs after "For example, the following does not work as intended because the package sub calls SG.SetRootDir first:"

Setogit
Setogit reviewed Jul 25, 2016
View reviewed changes
lib/payload.js
// this file has no tests so avoid refactor and ignore jshint for now
/*jshint ignore: start */
var serial,__hasProp = {}.hasOwnProperty;
var SG = require('strong-globalize');
Copy link

@Setogit Setogit Jul 25, 2016

Choose a reason for hiding this comment

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

Why not following strong-globalize coding guideline here?
https://www.npmjs.com/package/strong-globalize#autonomous-message-loading
Please study paragraphs after "For example, the following does not work as intended because the package sub calls SG.SetRootDir first:"

@Setogit
Copy link

Setogit commented Jul 25, 2016

For each JS module, please examine where `strong-globalize' is required and make sure the strong-globalize coding guideline is followed.
https://www.npmjs.com/package/strong-globalize#autonomous-message-loading
Paragraphs after "For example, the following does not work as intended because the package sub calls SG.SetRootDir first:"

@Setogit
Copy link

Setogit commented Jul 26, 2016

test please

@0candy
Copy link
Contributor

0candy commented Jul 26, 2016

ah sorry, missed that. Please fix Tetsuo's comment.

@0candy 0candy mentioned this pull request Jul 26, 2016
Merged
@rmg
Copy link
Member

rmg commented Jul 27, 2016

@slnode test please

@superkhau superkhau merged commit 6f0479d into master Jul 27, 2016
@superkhau superkhau deleted the globalization branch July 27, 2016 18:30
This was referenced Jul 27, 2016
Merged
Merged
@superkhau superkhau mentioned this pull request Aug 4, 2016
Merged
@superkhau superkhau mentioned this pull request Aug 16, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@0candy 0candy

Labels

feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@superkhau @0candy @Setogit @rmg @simonhoibm