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

Check pkg before trying to read name. #529

Closed
wants to merge 1 commit into from

Conversation

abuiles
Copy link
Member

@abuiles abuiles commented Apr 30, 2014

When trying to start a new project on a empty dir, init will fail, the reason is that project is a NULL_PROJECT which doesn't have pkg https://github.com/stefanpenner/ember-cli/blob/3e9323266e4b0922d8e519a1ac5444d796c2c98a/lib/models/project.js#L30

Steps to reproduce

mkdir /tmp/foo
cd /tmp/foo
ember init

@abuiles
Copy link
Member Author

abuiles commented Apr 30, 2014

@rjackson ^^

@abuiles
Copy link
Member Author

abuiles commented Apr 30, 2014

I might add some tests for this tomorrow.

@stefanpenner
Copy link
Contributor

should the NULL_PACKAGE of an empty pkg? That would likely be correct and follow this pattern

@rwjblue
Copy link
Member

rwjblue commented Apr 30, 2014

I think that we should be checking Project.prototype.name instead of Project.prototype.pkg.name, and our NULL_PROJECT can simply return undefined (which is what it is).

@@ -23,7 +23,7 @@ module.exports = new Command({
var installBlueprint = environment.tasks.installBlueprint;
var npmInstall = environment.tasks.npmInstall;
var bowerInstall = environment.tasks.bowerInstall;
var packageName = environment.project ? environment.project.pkg.name : path.basename(cwd);
var packageName = environment.project && environment.project.pkg ? environment.project.pkg.name : path.basename(cwd);
Copy link
Contributor

Choose a reason for hiding this comment

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

should be environment.project.isEmberCLIProject()

Copy link
Contributor

Choose a reason for hiding this comment

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

good point

Copy link
Member Author

Choose a reason for hiding this comment

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

@MajorBreakfast @stefanpenner this is not always true, at least not when running new, I went with that first and the command was breaking. I'll dig again later today.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, when running new, we are operating on the NULL_PROJECT which returns false for isEmberCLIProject(). As I mentioned below (in the normal comments) we should just ask the project for its name.

If NULL_PROJECT.prototype.name returned undefined, then all you would need is this (or a ternary if you prefered (which I do not)):

var packageName      = environment.project.name();

if (!packageName) {
  packageName = path.basename(process.cwd());
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@rjackson cool, I'll go with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm let's not introduce too many helper methods on Project.

It's just as short without project.name():

var packageName = path.basename(process.cwd());

if (environment.project.isEmberCLIProject()) {
  packageName = environment.project.pkg.name;
}

@stefanpenner
Copy link
Contributor

please ensure we have some coverage for this.

@abuiles
Copy link
Member Author

abuiles commented Apr 30, 2014

@rjackson I won't be able to look at this at least for the next 5-6 hours :( and people keep complaining about, if you want to give it a go, feel free to. cc @stefanpenner @MajorBreakfast

@twokul
Copy link
Contributor

twokul commented Apr 30, 2014

closing in favour of #546, thanks @abuiles

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.

5 participants