Allow users to skip the cleanup commit on restore #33
Conversation
That commit can cause problems when using ply with some automation systems.
There was a problem hiding this comment.
I like it. Just a few silly suggestions. Overall this seems like a nice win for using ply in https://github.com/openstack-infra/zuul
| @@ -295,7 +295,8 @@ def _ensure_name_and_email_set(self): | |||
| raise exc.GitConfigRequired('user.name') | |||
|
|
|||
| def restore(self, three_way_merge=True, commit_msg=None, | |||
| fetch_remotes=True, customize_commit_msg=False): | |||
| fetch_remotes=True, customize_commit_msg=False, | |||
| no_commit=False): | |||
There was a problem hiding this comment.
no_commit makes a bit of a double-negative when using logical operators. How about skip_commit?
There was a problem hiding this comment.
I actually had skip_commit first, but renamed it to no_commit because I like --no-commit as the CLI arg better (there's already some "skip" terminology in ply, and I'd rather avoid confusion). Also, it seems the same amount of negativity... 😐
| @@ -384,7 +385,9 @@ def restore(self, three_way_merge=True, commit_msg=None, | |||
| template = None | |||
|
|
|||
| # Determine whether we need to prompt the user to edit commit message | |||
| if commit_msg: | |||
| if no_commit: | |||
There was a problem hiding this comment.
See above for the rename suggestion of no_commit. After that, how about we skip the pass with:
if commit_msg and not skip_commit:
There was a problem hiding this comment.
Yeah, I was avoiding rewording the "else" at the end, but I can just change it to "elif not no_commit" I guess...
|
@rconradharris Any chance we can merge this change? :) |
That commit can cause problems when using ply with some automation systems.