strongloop / loopback-component-oauth2

oAuth 2.0 server for LoopBack
http://www.strongloop.com
Other
62 stars 63 forks source link

Add globalization #37

Closed loay closed 8 years ago

loay commented 8 years ago

Connect to strongloop/loopback/issues/2422

loay commented 8 years ago

@superkhau @0candy PTAL

jannyHou commented 8 years ago

From CI it fails due to

 assert.js:86
  throw new assert.AssertionError({
        ^
AssertionError: *** setRootDir: Intl dir not found under: /mnt/jenkins/workspace/loopback-component-oauth2/6d9b883b/lib

It might be fixed by rerun slt-globalize -e and alt-globalize -l each time before you commit your change.

Amir-61 commented 8 years ago

@loay

I had a quick review; I see lots of unrelated changes in this PR; please revert back unrelated changes like adding extra blank lines or removing blank lines,...

rmg commented 8 years ago

@slnode test please

loay commented 8 years ago

@superkhau @0candy PTAL again. Thanks.

0candy commented 8 years ago

@loay Please revert the debug statements.

loay commented 8 years ago

All Fixed. PTAL again Thanks @0candy

Amir-61 commented 8 years ago

@loay Basically whatever should not be translated like variable names you should skip them by using brackets: Example:

{{foo}}
0candy commented 8 years ago

You should revert this file lib/models/index.js and test/middleware/authorization.test.js

loay commented 8 years ago

Done more fixes. @superkhau @0candy PTAL again

@superkhau there is no bikeshedding. I am just trying to understand the logic behind it since changes in this repo are over 400 changes and I didn't want to start over. By the same logic of translation, I have actually fixed around 15-20 more words that the review did not catch.

superkhau commented 8 years ago

@loay Sounds good, if you feel it is a valid concern, then we should get @Setogit to answer that question as he is the correct person to ask. Please update us/globalization.md with your findings so we don't all end up asking the same question again.

jannyHou commented 8 years ago

@loay left two comments, others LGTM 👍

loay commented 8 years ago

@jannyHou @0candy Fixed both issues and rebased. Thanks

loay commented 8 years ago

@superkhau concerning all the unrelated ones, when you add the globalization declaration on top of each file, it automatically deletes/adds a line. They actually cancel each other out, since it is -/+, but they are still there somehow. Anyways there is a linting PR ready to go once this one is merged.

superkhau commented 8 years ago

@loay Bunch of nitpicks, but otherwise LGTM. As for the unrelated changes, maybe you should submit a PR running eslint, get that merged in, then rebase this PR so all those other edits will dissappear. Anyways, it's not a big deal since I looked at those changes during this review and they're all whitespace anyways. I'll leave it up to your discretion for what to do from here. :ship:

loay commented 8 years ago

@superkhau Thanks. I know it was a long review :)