Skip to content

Conversation

@davidcheung
Copy link
Contributor

@davidcheung davidcheung commented Jul 22, 2016

@bajtos stl-globalize uses file extensions to determine what gets globalized, I have updated the name to lb-ng.js(from lb-ng), is there any particular advantage or reasons we shouldn't change it? I can see it being more likely to be skipped by lint as well.

@0candy PTAL

/cc @Setogit

related to strongloop/loopback#2422

@davidcheung
Copy link
Contributor Author

@slnode test please

@bajtos
Copy link
Member

bajtos commented Jul 26, 2016

I have updated the name to lb-ng.js(from lb-ng), is there any particular advantage or reasons we shouldn't change it?

That's fine, I agree it's better to have a .js extension.

Just make sure the "scripts" entry in package.json on line 6 keeps working after rename.

When users install via npm install -g loopbsack-sdk-angular-cli, there should be lb-ng command available (no .js suffix in that command!).

ATM, we have

  "lb-ng": "bin/lb-ng"

I don't know if npm is smart enough to complete the missing extension. You may need to change that line to

  "lb-ng": "bin/lb-ng.js"

@davidcheung
Copy link
Contributor Author

@0candy PTAL

var generator = require('loopback-sdk-angular');
var SG = require('strong-globalize');
SG.SetRootDir(path.resolve(__dirname, '..'));
var g = SG();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be at the beginning of the file.

@0candy
Copy link
Contributor

0candy commented Jul 28, 2016

Changes LGTM. Please squash before merge.

note that `lb-ng` is changed to `lb-ng.js` because slt-globalize
checks files based on file extension
@davidcheung
Copy link
Contributor Author

@slnode test please

1 similar comment
@davidcheung
Copy link
Contributor Author

@slnode test please

@davidcheung davidcheung merged commit 25a57f4 into master Aug 5, 2016
@davidcheung davidcheung deleted the globalize branch August 5, 2016 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants