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

always return error on Execute and do no print it by default downstream. #147

Closed

Conversation

AntonioMeireles
Copy link
Contributor

@eparis, @spf13

i've found existing behaviour on command.Execute regarding error handling a bit incoherent, as it either prints the error by default while also returning it or doesn't even return it, printing it just.

attached patch attempts to fixe that. fwiw i've left previous behaviour of on error displaying (inline on Execute) usage info not because i think it slso makes too much sense but simply because lefting it (a minor issue) made simpler to get tests keep passing.

best regards

@eparis
Copy link
Collaborator

eparis commented Sep 10, 2015

I am not loving this. You made no actual change to the return of errors. Although you did make the code a touch more readable/obvious. Where you added a return would actual have fallen through and just returned the error. /me dislikes functions which use named return values...

So really all you did was remove the printing of errors in Execute(). Personally I think that is the right thin, but programs already have this printing, so they likely aren't doing it themselves. I'm scared to remove it....

@AntonioMeireles
Copy link
Contributor Author

eparis - fair points. trying to explaining myself better - all i'd like to have is the ability on 'my' side (i.e. above Execute()) of having full control on how to handle / display output in case of error. (right now, even if i want to tweak it, i always have to swallow whatever errored as is). actually, one could perhaps preserve exact same behaviour as today but with the error printing that is happening inside a default PostRun func/'template' that one could override at will. would that be ok to you ?

@eparis
Copy link
Collaborator

eparis commented Sep 10, 2015

I'm totally ok with some way for you to suppress the error output.

@spf13
Copy link
Owner

spf13 commented Sep 30, 2015

Where does this stand @AntonioMeireles ?

@spf13
Copy link
Owner

spf13 commented Nov 6, 2015

I believe this PR has been superseded by #169.. Closing. Open with comment if this is incorrect.

@spf13 spf13 closed this Nov 6, 2015
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.

3 participants