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

RunContext enhancements #570

Merged
merged 3 commits into from
Jun 5, 2014
Merged

RunContext enhancements #570

merged 3 commits into from
Jun 5, 2014

Conversation

blai
Copy link
Contributor

@blai blai commented May 29, 2014

  • Moves env and generator instatiation to _onReady so user have a chance to alter the environment to the a specific way and expect env and generator reflects such settings.
  • Emits ready event right before calling generator.run() to allow any
    last minute operation on the generator
  • Deprecating onEnd, instead emits 'end' event as generator.on('end')
  • Implement withGenerators() to mock generator dependencies
  • accepts optional second parameter in inDir as a callback function.
  • rename _holdExec to async
  • Update yeoman.io docs

@eddiemonge
Copy link
Member

can you update the commit message to be a bit more descriptive please?

@blai
Copy link
Contributor Author

blai commented May 29, 2014

I also recommend removing onEnd, and emit end event. But for backward compatibility sake, I kept that the way it is right now.

@eddiemonge
Copy link
Member

Before this change, this in the #onEnd() method referred to the generator. After these changes, this is undefined in #onEnd(). Perhaps missing a bind or call?

@SBoudrias
Copy link
Member

@blai This is still a new system, feel free to remove onEnd and emit an end event. We initially didn't use an event for the end as we didn't used them, but now that there's a ready event, it makes sense.

Also, could you send a follow-up PR to yeoman/yeoman.io updating the related documentation?

@SBoudrias
Copy link
Member

Before this change, this in the #onEnd() method referred to the generator. After these changes, this is undefined in #onEnd(). Perhaps missing a bind or call?

I think I'd prefer the generator instance to be passed as an explicit parameter rather than it being linked to the this value. @blai would you want to take care of this change too?

@blai
Copy link
Contributor Author

blai commented Jun 2, 2014

@SBoudrias NP, I'll take care of it.

@blai
Copy link
Contributor Author

blai commented Jun 4, 2014

@eddiemonge @SBoudrias
I try to consolidate changes to #564, #565 and #573 all in this one PR, if that is ok with you guys. I have also started a PR for yeoman.io repo to update the docs, but I guess now I also need to document the withGenerator syntax.

Please comment

@blai blai changed the title fixes #565 RunContext enhancements Jun 4, 2014
it('accepts optional `cb` to be invoked with resolved `dir`', function (done) {
var ctx = new RunContext(this.Dummy);
var cb = sinon.spy(function () {
assert(cb.calledOnce);
Copy link
Member

Choose a reason for hiding this comment

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

you can use sinon.assert here:

sinon.assert.calledOnce(cb);
// etc

It output prettier (more useful) assertion reports :)

@SBoudrias
Copy link
Member

Ideally each feature should hold in it's own focused commit. But I understand if you don't want to edit your history.

Also, can you keep the object literals on one line when they doesn't span more than 80-90 chars longs?

@eddiemonge
Copy link
Member

@blai no twitter?

… a chance to alter

the environment to the a specific way and expect `env` and `generator` reflects such
settings.

- Emits `ready` event right before calling `generator.run()` to allow any
last minute operation on the `generator`

- Deprecating `onEnd`, instead emits 'end' event as `generator.on('end')`

- Implement `withGenerator()` to mock generator dependencies

- accepts optional second parameter in `inDir` as a callback function.

fixes yeoman#564, yeoman#565, yeoman#573
@kidwm
Copy link

kidwm commented Jun 5, 2014

@blai Cheers!

this.env = yeoman();

this._dependencies.forEach(function (d) {
if (d instanceof Array) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you use _.isArray?

@SBoudrias
Copy link
Member

@blai awesome works! I'm feeling pretty good about merging it.

The only thing I'd like is for the inDir callback to allow the var done = this.async() pattern in case someone need to do some async action. But this can come in a later PR.

Brian Lai added 2 commits June 4, 2014 22:47
…nerators into an env.

(also change to use _.isArray for array type check)
…ould use it to perform async tasks.

Rename `_onReady` to `_run` (since it is not really "ready" yet when it is called)
@blai
Copy link
Contributor Author

blai commented Jun 5, 2014

@SBoudrias adopting var done = this.async() pattern is really just renaming _holdExec to async, but added a unit test to prove it is the case.

@SBoudrias
Copy link
Member

@blai oh right, clever 🎆

Merging!

SBoudrias added a commit that referenced this pull request Jun 5, 2014
RunContext enhancements
@SBoudrias SBoudrias merged commit 80ac089 into yeoman:master Jun 5, 2014
@SBoudrias
Copy link
Member

You'll update your PR on Yeoman.io for the doc?

blai pushed a commit to blai/yeoman.io that referenced this pull request Jun 5, 2014
eddiemonge pushed a commit to yeoman/yeoman.io that referenced this pull request Jul 8, 2014
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.

4 participants