Add an output_nostrip kwarg to Git.execute#77

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

znerol wants to merge2 commits intogitpython-developers:masterfromznerol:feature/master/cmd-output-nostrip

Closed

Add an output_nostrip kwarg to Git.execute#77

znerol wants to merge2 commits intogitpython-developers:masterfromznerol:feature/master/cmd-output-nostrip

Conversation

Copy link

Thanks for the fix.
One thing that you might consider changing is the double-negation. Thus you could call the parameteroutput_strip and default it totrue.
As you have created the fix to make diff work better, it wold be great to have a test case which verifies this functionality.

Copy link

Author

znerol commentedOct 16, 2012

One thing that you might consider changing is the double-negation.

I tried this but it does not feel right for me. IMHOGit.execute should somehow behave like invoking a command line tool. Many cli tools have--no-xy-style options to turn off features which normally are turned on. Just take a look at some randomman-pages (e.g.man man). Maybe it's just me but I think the double negation works better in this case.

I try to come up with some test cases.

Add unittest for output_nostrip kwarg

28375f5

Copy link

Member

Byron commentedOct 18, 2012

Thanks for the unit test, its much appreciated.

Besides that, I don't think one can compare a CLI with an API. The--no-* or--disable-* options might also be paired with--enable-* or--* counterparts. Those usually set nothing more than a flag to true or false which is then handled in code.

In APIs I consider it bad practice to negate something in the first place, and then have double-negations in the code likeif not no_strip. Its much nicer to have to have thestrip option with a True default value, and then check withif strip: in code.