Add globalization #37
Conversation
|
@superkhau @0candy PTAL |
|
|
||
| !intl/ | ||
| intl/* | ||
| !intl/en/ |
There was a problem hiding this comment.
It should be ordered like this: (Thats my two cents)
build/
reports/
!intl/
intl/*
!intl/en/
# Node.js
node_modules/
npm-debug.log
# Mac OS X
.DS_Store
There was a problem hiding this comment.
Let's not bikeshed here. It's going to work wherever you add it (and there is no convention for .gitignores across all LoopBack projects yet -- maybe we should come up with one and then fix them all at that time).
build/
reports/
!intl/
intl/*
!intl/en/
This part isn't alphabetized correctly in your example anyways.
There was a problem hiding this comment.
Let's not bikeshed here.
Sure!
But basically I did not mean alphabetical order; I mean order based on the categories
...
# Node.js
...
# Mac OS X
...
There was a problem hiding this comment.
IMO, if we actually went with it, it should be a combination of both. Categories and alphabetically inside each category. Anyways, enough 🚲 🏠 ;)
|
From CI it fails due to It might be fixed by rerun |
| var layer = stack[idx++]; | ||
| if (!layer) { return cb(); } | ||
|
|
There was a problem hiding this comment.
Nitpick: Please revert back unrelated changes everywhere.
There was a problem hiding this comment.
@Amir-61 : to save you time .. that’s not unrelated code .. that was pushed by the 2 lines of globalization on top, then by linting .. Not mine
There was a problem hiding this comment.
@loay I see, probably we should have added eslint which could take care of all of these things before...
|
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,... |
|
|
||
| var SG = require('strong-globalize'); | ||
| var g = SG(); | ||
| /** |
There was a problem hiding this comment.
These two lines have to be first in the file.
|
@slnode test please |
|
|
||
| req.oauth2.redirectURI = redirectURI; | ||
|
|
|
@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. |
| // restricted by GSA ADP Schedule Contract with IBM Corp. | ||
|
|
||
| var SG = require('strong-globalize'); | ||
| var g = SG(); |
|
@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. 🚢 |
|
@superkhau Thanks. I know it was a long review :) |
6 participants
Connect to strongloop/loopback/issues/2422