Closed dparfrey closed 12 years ago
Thank you for using exec-plan. Currently, that is not possible. That was a feature I wanted to put in initially, but saved for a later date.
My thought on how to accomplish this would be to provide a configuration option object to the ExecPlan constructor, with 2 options (for the time being): 'autoPrintOut' and 'autoPrintErr'. These options would be looked up whenever stdout or stderr would be printed automatically during plan execution. The default values would be 'true'.
I could personally get that in later today. I am on PST. My workday will be over around 7pm PST.
That would be very cool! No special hurry, I'll be writing my utility for a few days, so I can easily proceed, knowing it'll be available by the time I'm done.
Thanks! Exec-plan is very useful for this utility. We're planning to make this an open source git-based deployment utility, btw.
Thanks for the update! That sounds like a cool project. Hope everything goes well with that. I'm working on getting the feature in, and I'll keep you posted if anything spills over past the weekend; however, there is no reason it should take that long.
Quick heads up: The node.js 'events' module has a conflict with events named "error"; specifically, it throws an error if the event is named "error". Therefore, I have renamed the 'error' event to 'execerror'. I apologize for the inconvenience if you have already written code against the 'error' event. The good news is that I am done with the functionality; I am writing unit tests now to make sure everything goes well, and tomorrow (Sunday), I should have everything registered in the npm registry with a new branch added to the github repo.
Ok, I am finished with the feature. I have made this feature set the 'v0.0.2' release. It is tagged, and can be downloaded directly as zip or tarball. Also, I have registered this release as 0.0.2 in the npm registry, so if you update your package.json to use "0.0.2" for exec-plan in the dependencies portion, you should be fine.
Also, I updated the main documentation to match the updates. Thank you for using exec-plan, and I hope everything goes well with your project!
I will keep the issue open for a few days in case anything goes wrong.
No problem changing the event name, it's a minor change. Thanks for the quick action on this, I'll let you know when I'm all upgraded.
Looks like your changes are working fine. Thanks again!
At the risk of overstaying my welcome: is it possible for exec-plan to ignore errors and keep running the plan? For example, I want to 'git checkout master' at the start of my plan, but if it's already the current branch, the rest of the plan doesn't run.
No problem! That does make sense; it should be configurable to allow continued execution if errors occur. Also, just thinking outloud, it would probably make sense to allow the 'execerror' event handler to return false to stop the plan, even if it's configured to keep running on error. What do you think?
Yeah, essentially, you're setting the default to continue, but with a stop override. I like it! But does it make more sense to stop execution from an individual error handler? Then it's easier to identify the commands I care about errors on. But you'd need a different flag than true/false, I think.
My thought was to allow the individual error handler to return false to individually stop the plan. And if an error handler doesn't return false, then the configuration set up via the constructor would be checked to determine if execution should continue.
Oh wait, I forgot that the individual error handlers are already returning false to stop the 'error' event from firing.
I will think about this today, and make sure that it doesn't start getting hacky :)
Here's what I'm thinking:
From a conceptual standpoint, in my opinion, it makes more sense for "returning false" in the individual error handlers to stop the execution plan, rather than preventing the 'execerror' event from firing. I think it would make sense to remove the feature of stopping the 'execerror' event from firing, and re-think that at a later date. What do you think?
How about this? If an individual error handler returns true, then the execution plan continues and the 'execerror' event doesn't fire. If it returns false, then the execution plan stops and the 'execerror' event fires. In other words, I don't want to run the 'execerror' handler unless I'm bailing out on the plan early.
Seems to make sense to me. How 'bout you?
I re-read what you said a couple of times, and that's actually a really elegant way to deal with the problem! I also think it's a good idea that if the 'execerror' event handler returns true, it should have the same behavior (in case the user wants to have a general handling mechanism for any case that doesn't have an individual handler). At the current time, I think this would be the proposal for list of changes:
1.) Provide a configuration option in the constructor that sets the general policy of whether to continue execution on errors if error handlers (individual and 'execerror') don't specifically control this behavior themselves.
2.) Update individual error handlers with following behavior: a.) if it returns true or false, it prevents the 'execerror' event from firing; otherwise, the general policy set in the constructor is followed b.) if it returns true, plan should continue irrespective of general policy c.) if it return false, plan should stop irrespective of general policy
3.) Update 'execerror' event handler logic to use following behavior: a.) if it returns true, plan should continue irrespective of general policy b.) if it returns false, plan should stop irrespective of general policy
Does this sound good to you?
It sounds perfect! It'll give us all the flexibility we'll ever need.
Great! I'll try to finish that by the end of the week. What type of timetable are you working with?
I've got lots more code to write, so no problem. I'll work around it for now and clean things up later. Thanks!
Great, I'll let you know once it is complete. And thanks for helping me make this better!
Quick update: It turns out that the node.js events module does not give access to the return value of event handler functions. I'm sure there could be some plumbing that could be put in place to get around this, but I don't want to make this module that complex at this stage. Therefore, in the interest of time, the 'execerror' event handlers will not have the ability to override the general "continueOnError" policy at this time. However, I will implement the ability for individual error handlers to override the policy as we discussed.
I agree. I'm not sure I need the execerror override, anyway. Setting an overall option with an individual command override seems plenty good enough.
Quick update: the error handler functionality exposed a bug in how I wire up error handlers. I think I should be able to fix it, though, by the end of the weekend or earlier.
Thanks for the update. Finding and fixing bugs is always good!
Ok, I am finished with the feature. I have made this feature set the 'v0.0.3' release. It is tagged, and can be downloaded directly as zip or tarball. Also, I have registered this release as 0.0.3 in the npm registry, so if you update your package.json to use "0.0.3" for exec-plan in the dependencies portion, you should be fine.
Several test cases are in place for this in the 'test' directory under the 'errorHandlers' group of unit tests.
The bug that is associated with error handling wiring is handled in Issue #2. The bug is now fixed, and test cases are in place for that.
Also, I updated the main documentation to match the updates. I will keep this issue open until you have tried it out in your project. Thank you for using exec-plan, and I hope everything goes well with your project!
The doc looks awesome! I'll try to get it incorporated into my stuff in the next few days and let you know (but I'm sure it'll work great.)
Everything seems to be working, at least all the existing code that I converted. I have one more new set of commands to write, but that's it. This new plan will use the individual function override for "continueOnError", I didn't need it on the other plans. Thanks again for all your help!
Awesome, great to hear! I will keep this issue open for the rest of the week. Let me know if anything goes wrong, although, I have enough test cases to be confident it will work! Cheers!
I'm done with all the exec-plan code I need to write (for now). Everything's working perfectly! Moving on to other things, you can close the issue whenever you want. Once again... Thanks!
Great to hear! And thank you for helping with this project. I hope everything goes well with your project!
I'm using exec-plan to exec a series of git commands. For example, I want to get the list of remotes defined (with git remote -v). I need to process the output in the next function in the plan, so I need stdout, but I don't want it to show in my terminal window.
Is this possible, and how could I do this?
Thanks!