Use the set app name from bower.json #462
Conversation
| // - args if supplied | ||
| // - Current working directory | ||
| try { | ||
| this.appname = require(path.join(process.cwd(), 'bower.json')).name; |
There was a problem hiding this comment.
bower.json first, then package.json
| this.appname = require(path.join(process.cwd(), 'package.json')).name; | ||
| } | ||
| catch (e) { | ||
| this.appname = this.args[0] || path.basename(process.cwd()); |
There was a problem hiding this comment.
I don't think we should use the arguments here. It won't work for subgenerators who are expecting other arguments. And it'll break behavior for generators expecting a first arguments (for example, in Generator-BBB we ask a file path as first arg).
There was a problem hiding this comment.
Would you run a sub-generator in a project without a bower or package json? If so how/where should I get the app name from argv when the project is first scaffolded?
There was a problem hiding this comment.
I just don't think we should use argv by default. If a generator want to use it, then they can implement it themselves. Otherwise, using the folder name looks good enough for me.
|
I think this would all be cleaner if you'd exported the logic inside a method ( That'd make tests way cleaner as you wouldn't need to instantiate a new Generator inside each tests, and it'd make the constructor function code easier to follow. |
|
@SBoudrias I agree, I'll have another pass on it tomorrow. I didn't have much time to look at the rest of the code today but if you could answer the questions on the outdated diffs that would help. Cheers. |
|
@SBoudrias how's this looking now? |
|
The PR currently still has some merge conflicts that need to be resolved :) |
|
@addyosmani I'll rebase and squash some of the erroneous commits if you guys are happy with what is here |
|
LGTM for a merge once @SBoudrias has given his sign-off. |
|
Awesome! Thanks a lot |
Use the set app name from bower.json
Follow on from the discussions here: yeoman/generator-backbone#187