Skip to content
This repository

Disable Stdout Terminal output #1

Closed
dparfrey opened this Issue September 20, 2012 · 30 comments

2 participants

Dave Parfrey Ryan Self
Dave Parfrey

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!

Ryan Self
Owner

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.

Dave Parfrey

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.

Ryan Self
Owner

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.

Ryan Self
Owner

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.

Ryan Self
Owner

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!

Ryan Self ryan-self closed this September 22, 2012
Ryan Self ryan-self reopened this September 22, 2012
Ryan Self
Owner

I will keep the issue open for a few days in case anything goes wrong.

Dave Parfrey

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.

Dave Parfrey

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.

Ryan Self
Owner

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?

Dave Parfrey

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.

Ryan Self
Owner

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.

Ryan Self
Owner

Oh wait, I forgot that the individual error handlers are already returning false to stop the 'error' event from firing.

Ryan Self
Owner

I will think about this today, and make sure that it doesn't start getting hacky :)

Ryan Self
Owner

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?

Dave Parfrey

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?

Ryan Self
Owner

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?

Dave Parfrey

It sounds perfect! It'll give us all the flexibility we'll ever need.

Ryan Self
Owner

Great! I'll try to finish that by the end of the week. What type of timetable are you working with?

Dave Parfrey

I've got lots more code to write, so no problem. I'll work around it for now and clean things up later. Thanks!

Ryan Self
Owner

Great, I'll let you know once it is complete. And thanks for helping me make this better!

Ryan Self
Owner

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.

Dave Parfrey

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.

Ryan Self
Owner

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.

Dave Parfrey

Thanks for the update. Finding and fixing bugs is always good!

Ryan Self ryan-self referenced this issue from a commit September 28, 2012
Ryan Self Issue #1, #2: Complete feature to introduce "error continuing" policy…
…, with the ability for individual error handlers to override that policy. Also, fix bug originally blocking this feature. This bug entailed the individual error handlers not being called for corresponding commands.
0a8897c
Ryan Self
Owner

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!

Dave Parfrey

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.)

Dave Parfrey

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!

Ryan Self
Owner

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!

Dave Parfrey

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!

Ryan Self
Owner

Great to hear! And thank you for helping with this project. I hope everything goes well with your project!

Ryan Self ryan-self closed this October 02, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.