PR on issue #742 withLocalConfig - basic tests & implementation

[gruppjo]

As discussed on issue #742 I created a PR:

Here's what I've done so far:

test/run-context.js

write unit test for withLocalConfig() method

lib/test/helpers.js

write a mockLocalConfig() function, that uses the generator's store's default() method to provide the config. This seemed an easy way to implement it, however I'm not sure whether it's a good idea to do so.

lib/tests/run-context.js

extend the RunContext constructor, its _run() method and added a withLocalConfig() method to put everything together.
Notice how helper.mockLocalConfig()only gets called when withLocalConfig() was called? I needed to restrict this, since otherwise one of the tests would try to create a .yo-rc.json in / (root directory) and obviously would fail (I don't know why this happens though...).

Please let me know what you think.

[coveralls]

Coverage increased (+0.02%) to 97.18% when pulling f8b7f3e on gruppjo:issue-742-withConfig into 461a181 on yeoman:master.

[hemanth]

👍 //cc @SBoudrias

[addyosmani]

👍

[SBoudrias]

Hey @gruppjo, thanks for the PR. This look mostly good.

The only thing is I think we should create a .yo-rc.json file and let the generator consume it rather than calling default() to set the config. This will replicate the real behavior and reduce the risk of unexpected behavior for our user.

[gruppjo]

Thanks for your feedback!
I thought about this too but I wasn't quite sure what would be the best way to do it. This is how I'd do it:

• call set() on Storage for every key value pair in the localConfig object

Since the generator isn't run until later and calling set() also writes the file implicitly.

[SBoudrias]

I don't want us to call any method on the generator. Let's not make any assumption on the inner working of the generator, let's just create the environment it's going to use externally. That's why I want to create the .yo-rc.json file first and let the generator consume it in whatever way they need.

[gruppjo]

Thank you for your input! Just to make sure I understand this correctly:

The helpers.mockLocalConfig() would create a .yo-rc.json file in the test directory without using any generator code. I think I'll be able to do this, although I'm not quite sure how to get the path of the test directory yet. I'll try to find out.

However I have a concern: when manually creating the yo-rc.json it will be necessary to provide the generator's name with the withLocalConfig() call. So instead of writing (as it is in the current implementation):

helpers.run(path.join(__dirname, '../app'))
  .withPrompt(someAnswers)  // answer prompts
  .withLocalConfig({
    some: true,
    data: 'here'
   })
  .on('end', done);

You'd have to pass in the generator's name as well. Which I think is a little counter intuitive, isn't it?

helpers.run(path.join(__dirname, '../app'))
  .withPrompt(someAnswers)  // answer prompts
  .withLocalConfig({
    'generator-super-awesome': {
      some: true,
      data: 'here'
    }
   })
  .on('end', done);

This feels a little bloated (implementation & usage) to me. What do you think?

[coveralls]

Coverage increased (+0.17%) to 97.33% when pulling 3726d30 on gruppjo:issue-742-withConfig into 461a181 on yeoman:master.

[coveralls]

Coverage increased (+0.17%) to 97.33% when pulling 9634399 on gruppjo:issue-742-withConfig into 461a181 on yeoman:master.

[gruppjo]

Hi, I was able to do some more work on this (sorry for the two separate commits, I was in a bit of a hurry).

The code now creates a .yo-rc.json before the generator is created. This results in the drawback that one has to pass in the name of the generator in the config as well. I thought about reading the name of the generator from the package.json, but I didn't have enough time to figure out how to do this. Since the generator isn't created and generator.rootGeneratorName() cannot be called yet.

I'm not so sure about this one... Let me know what you think.

[SBoudrias]

Hey @gruppjo I've rethought this over and I think your first solution calling config.default() will be good enough. Creating the config after the generator is instantiated won't work, and creating it before is error prone because we won't know the generator name at that time.

Mind reverting your changes to use your first solution?

[gruppjo]

@SBoudrias, I absolutely don't mind :) I'll get to it as soon as possible!

[coveralls]

Coverage increased (+0.26%) to 97.42% when pulling 4c4da61 on gruppjo:issue-742-withConfig into 461a181 on yeoman:master.

[gruppjo]

I reverted the changes, but not using git revert but by creating a new commit. Is that ok?
However the build on Travis fails although the tests run just fine. Did I do something wrong? - Apparently the build for node 0.12 and iojs is failing.

[SBoudrias]

Looks like a build error, I restarted them and it's passing.

[gruppjo]

👍 great!