Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Add an output_nostrip kwarg to Git.execute #77

wants to merge 2 commits into from

Conversation

znerol
Copy link

@znerol znerol commented Oct 12, 2012

The last line of the output normally is stripped if it is empty. If output_nostrip is True, this behavior is disabled. This can be important when constructing patches e.g. using the diff command.

The last line of the output normally is stripped if it is empty. If
output_nostrip is True, this behavior is disabled. This can be important
when constructing patches e.g. using the diff command.
@Byron
Copy link
Member

Byron commented Oct 16, 2012

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

@znerol
Copy link
Author

znerol commented Oct 16, 2012

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

I tried this but it does not feel right for me. IMHO Git.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 random man-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.

@Byron
Copy link
Member

Byron commented Oct 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 like if not no_strip. Its much nicer to have to have the strip option with a True default value, and then check with if strip: in code.

@znerol
Copy link
Author

znerol commented Oct 18, 2012

Following up in #79

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants