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

BUGFIX Fixes #3472 Check for 'usePods' instead of 'pod'. #3481

Merged
merged 4 commits into from
Mar 10, 2015

Conversation

jankrueger
Copy link
Contributor

Fixes #3472 by checking for 'options.usePods' instead of 'options.pod'

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2015

@trabus - 👍 / 👎 ?

@trabus
Copy link
Contributor

trabus commented Mar 9, 2015

usePods is the wrong value to be checking for. The options.pod value is passed on from the generate command, and should be present in locals, if it's not that's a bug that needs to be investigated.

@trabus
Copy link
Contributor

trabus commented Mar 9, 2015

@jankrueger So the fix is actually just as simple, but belongs in the blueprint model here. When the value for usePods is present, we're inverting the value of options.pod, but not setting the value back on options.pod. We need to fix the options.pod value to be the correct value when usePods is true.

We have two options.
First would be to mutate the options.pod value from the blueprint model (quickest option)

// lib/models/blueprint.js install and destroy methods
var pod = (options.settings && options.settings.usePods) ? !options.pod : options.pod;
this.pod = options.pod = pod;

The second would be to check for usePods earlier in the generate and destroy commands, and set the value there. (cleaner option, handled upstream, preferred)

// lib/commands/generate.js and lib/commands/destroy.js
 if(this.settings && this.settings.usePods) {
   commandOptions.pod = !commandOptions.pod;
 }
 var taskOptions = merge(taskArgs, commandOptions || {});

// lib/models/blueprint.js install and destroy methods
this.pod     = options.pod;

We should probably add a test for this as well, you should be able to add one in tests/acceptance/pods-generate-test.js

@jankrueger
Copy link
Contributor Author

@trabus Thanks for clarification!
There seems to be a test in place that is failing. I will fix that hopefully tonight with this pr.

@trabus
Copy link
Contributor

trabus commented Mar 9, 2015

@jankrueger The existing tests should be fine after this fix, just be sure to revert the change in the component blueprint. I was suggesting we add an additional generate test with usePods set.

Regarding additional tests, you should be able to copy this one, and change the values being passed. You'll just need to adjust what the expected output should be so it matches what a component generates.

@jankrueger
Copy link
Contributor Author

@trabus I implemented your preferred version and changed the commandOptions in the generate/destroy command. The failing test is now passing and i added two more tests as suggested.

@trabus
Copy link
Contributor

trabus commented Mar 9, 2015

@jankrueger Looking good! 👍

Can @rwjblue or @stefanpenner review, please?

@rwjblue
Copy link
Member

rwjblue commented Mar 10, 2015

LGTM

rwjblue added a commit that referenced this pull request Mar 10, 2015
BUGFIX Fixes #3472 Check for 'usePods' instead of 'pod'.
@rwjblue rwjblue merged commit 07bed6f into ember-cli:master Mar 10, 2015
@rwjblue
Copy link
Member

rwjblue commented Mar 10, 2015

@jankrueger / @trabus - Thanks for working together on this!

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.

[0.2.0] Component blueprint imports layout module regardless of pod structure
3 participants