Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Deprecate default of Git.show(strip_newline_in_stdout=False)#1948
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks a lot for contributing.
I think with a few minor modifications this PR can be merged.
| @@ -1210,6 +1210,14 @@ def execute( | |||
| if self.GIT_PYTHON_TRACE and (self.GIT_PYTHON_TRACE != "full" or as_process): | |||
| _logger.info(" ".join(redacted_command)) | |||
| if strip_newline_in_stdout and command[:2] == ["git", "show"]: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"git" shouldn't be hard-coded, and I am pretty sure there is a variable for this somewhere.
| @@ -1210,6 +1210,14 @@ def execute( | |||
| if self.GIT_PYTHON_TRACE and (self.GIT_PYTHON_TRACE != "full" or as_process): | |||
| _logger.info(" ".join(redacted_command)) | |||
| if strip_newline_in_stdout and command[:2] == ["git", "show"]: | |||
| warnings.warn( | |||
| "Git.show() has strip_newline_in_stdout=True by default, which probably isn't what you want and will " | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| "Git.show() has strip_newline_in_stdout=True by default, which probably isn't what you want andwill " | |
| "Git.show() has strip_newline_in_stdout=True by default, which probably isn't what you want andmay " |
| return r | ||
| @with_rw_directory | ||
| def test_warn_when_strip_newline_in_stdout(self, rw_dir): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's put the additional assertion done intest_do_not_strip_newline_in_stdout here so that it's clearer what's under test: The deprecation warning ifgit show is used andstrip_newline_in_stdout=True. This way,test_do_not_strip_newline_in_stdout can be removed. I'd also hope that the assertion then can be changed to use the default value forstrip_newline_in_stdout, which is probablyTrue, the cause of the confusion in this case.
As per our discussion in#1377.
Also fix a few tests that were broken by my git being configured to create new repos with
mainas the initial branch, so that all my tests pass.