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

Grouped Queue #475

Merged
merged 3 commits into from
Jan 22, 2014
Merged

Grouped Queue #475

merged 3 commits into from
Jan 22, 2014

Conversation

SBoudrias
Copy link
Member

Follow up PR to issue #433

I still need to:

  • Run the prototype methods as part of their relevant queue group
  • Fix issue where Grouped-Queue will always set default as the last queue (I'll change that in the plugin)
  • Add a "run only once" option to Grouped-Queue so install is not called for every composed Generator

ATM, I add #installDependencies() in the end queue inside the #run() method. I wonder if this should not be taken care of inside #installDependencies() itself? (That would prevent issues where npm and bower are called multiple times if old Generators still call #installDependencies() manually).

Extract installation tests in its own module
@addyosmani
Copy link
Member

I would be in favour of adding it directly inside. If there's a chance npm/bower can get repeatedly called we want to avoid that.

Now Conflicter#resolve is only called once after all method ran
Now Base#installDependencies is called once at the end unless
--skip-install is passed as option. (This required some changes in test so
not every Generator test spawned an install process)
@SBoudrias
Copy link
Member Author

I would be in favour of adding it directly inside. If there's a chance npm/bower can get repeatedly called we want to avoid that.

Updated that. Note though that only #installDependencies() get queued. As npm and bower install methods could be passed special parameters (not only based on package.json content), it wasn't really possible to queue them only once automatically.

I also added a commit so tasks having the name (or hash) of a queue are runned in that queue.

I'll probably refactor the #run() method sometime in the future as it does 100 lines long ATM. But for this PR, it should be good to go!

@SBoudrias
Copy link
Member Author

I was thinking about this and there's probably a bug inside it. I'm pretty sure the first prototype method will always run first no matter what (grouped-queue starting running as soon as the first task is appended).

So, I guess I'll need to order how tasks are added to the runLoop. I'll refactor the run method to be lighter at the same time.

@addyosmani
Copy link
Member

Given the significance of that particular queue issue, let's wait until you've had a chance to confirm the issue and fix before moving ahead.

@SBoudrias
Copy link
Member Author

Ok, finally I was scared for nothing, the weren't any bug :) I added a test ensuring this though, and while looking I found some edge case and easy refactoring.

It is now cleaner and stronger!

addyosmani added a commit that referenced this pull request Jan 22, 2014
@addyosmani addyosmani merged commit 47be06f into yeoman:master Jan 22, 2014
@SBoudrias SBoudrias deleted the group-queue branch January 22, 2014 22:23
@addyosmani
Copy link
Member

Thanks for the extra time testing and reviewing the implementation, Simon!

oh_yeah_obama

@SBoudrias
Copy link
Member Author

We'll need good beta testing on this one!

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