Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed missing package name in generated cli.js #132

Closed
wants to merge 2 commits into from

Conversation

petersandor
Copy link

this.options.name was undefined since CLI generator doesn't have any name option specified.

@SBoudrias
Copy link
Member

Please add a unit test. We want to make sure this won't break if someone run yo node --cli and yo node:cli.

@petersandor
Copy link
Author

Thanks, I added a basic test that compares whether the name variable contains anything. By the way, seems like yo node --cli just runs the main app generator - even before my changes.

Checks the presence of package name in lib/cli.js, see issue yeoman#133
@SBoudrias
Copy link
Member

yo node --cli will run the main generator and then compose the node:cli generator

@@ -21,7 +21,7 @@ module.exports = generators.Base.extend({
this.fs.copyTpl(
this.templatePath('cli.js'),
this.destinationPath('lib/cli.js'), {
pkgSafeName: _.camelCase(this.options.name),
pkgSafeName: _.camelCase(this.appname),
Copy link
Member

Choose a reason for hiding this comment

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

If we're not using the --name anymore, we should remove it.

Copy link
Author

Choose a reason for hiding this comment

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

Actually using --name works. I wasn't aware of that, my mistake. I did not provide and it still ran without errors, I guess some check for that might be useful.

edit: it is not mentioned in --help and I looked there, nevermind I guess this PR can be closed. Thanks for quick help.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it should be close. We have (or should have) access to the app name without requiring someone to manually pass an option.

The interface will be better if we can just figure that value out without user input.

@petersandor
Copy link
Author

I'm closing this PR since there is a possibility to provide --name to avoid the issue (#133), as a new user I was not aware of it, since it wasn't mentioned in --help.

I think two things need to be done:

  1. Mention --name in subgenerator --help
  2. If --name isn't provided, use this.appname as a default

I will leave the issue open for discussion. Thanks @SBoudrias for the quick feedback. 👍

@petersandor petersandor closed this Jul 8, 2015
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.

None yet

2 participants