Skip to content

Add globalization#37

Merged
loay merged 1 commit intomasterfrom
globalization
Jul 28, 2016
Merged

Add globalization#37
loay merged 1 commit intomasterfrom
globalization

Conversation

@loay
Copy link
Copy Markdown
Contributor

@loay loay commented Jul 26, 2016

@loay loay added the #review label Jul 26, 2016
@0candy 0candy mentioned this pull request Jul 26, 2016
Closed
63 tasks
@loay
Copy link
Copy Markdown
Contributor Author

loay commented Jul 26, 2016

@superkhau @0candy PTAL

Amir-61
Amir-61 reviewed Jul 26, 2016
View reviewed changes
Comment thread .gitignore

!intl/
intl/*
!intl/en/
Copy link
Copy Markdown
Member

@Amir-61 Amir-61 Jul 26, 2016

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@superkhau superkhau Jul 27, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@Amir-61 Amir-61 Jul 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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
...

Copy link
Copy Markdown
Contributor

@superkhau superkhau Jul 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, if we actually went with it, it should be a combination of both. Categories and alphabetically inside each category. Anyways, enough 🚲 🏠 ;)

@jannyHou
Copy link
Copy Markdown

jannyHou commented Jul 26, 2016

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
Amir-61 reviewed Jul 26, 2016
View reviewed changes
Comment thread lib/server.js
var layer = stack[idx++];
if (!layer) { return cb(); }

Copy link
Copy Markdown
Member

@Amir-61 Amir-61 Jul 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Nitpick: Please revert back unrelated changes everywhere.

Copy link
Copy Markdown
Contributor Author

@loay loay Jul 26, 2016

Choose a reason for hiding this comment

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

@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

Copy link
Copy Markdown
Member

@Amir-61 Amir-61 Jul 26, 2016

Choose a reason for hiding this comment

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

@loay I see, probably we should have added eslint which could take care of all of these things before...

@Amir-61
Copy link
Copy Markdown
Member

Amir-61 commented Jul 26, 2016

@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,...

0candy
0candy reviewed Jul 26, 2016
View reviewed changes
Comment thread lib/exchange/clientCredentials.js

var SG = require('strong-globalize');
var g = SG();
/**
Copy link
Copy Markdown
Contributor

@0candy 0candy Jul 26, 2016

Choose a reason for hiding this comment

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

These two lines have to be first in the file.

@rmg
Copy link
Copy Markdown
Member

rmg commented Jul 27, 2016

@slnode test please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@loay @jannyHou @Amir-61 @rmg @0candy @superkhau