Implementation for hook to define phases of playback during git commands #122
Conversation
Wiseqube
changed the title
|
This is running as expected so far, we'll of course have to have the correct script to make use of this functionality! |
Wiseqube
force-pushed
the
playback-hook
branch
from
325a5ec to
f28008d
Compare
perfact/zodbsync/subcommand.py
Outdated
| @@ -150,12 +151,80 @@ def gitexec(func): | |||
| - play back changed objects (diff between old and new HEAD) | |||
| - unstash | |||
| """ | |||
| def _playback_paths(self, paths): | |||
There was a problem hiding this comment.
Why do we need this embedded function? Could we pull this out of def gitexec?
There was a problem hiding this comment.
Shouldn't be a problem, I think this was due to the iterative development. Basically started in the wrapper but soon moved it out but kept it contained inside of gitexec
perfact/zodbsync/subcommand.py
Outdated
| dryrun=dryrun, | ||
| ) | ||
|
|
||
| if not dryrun and phase_cmd and os.path.isfile(phase_cmd): |
There was a problem hiding this comment.
Please turn this around and by using a negated condition and continue.
|
@viktordick I've adjusted the files according to your review, I also added an entry in the readme, the config and the changelog! Additionally, which is mainly due to the fact that I saw a deprecation warning during the running of the checks I've updated the major versions of the github actions used in our workflows! |
| if playback_hook and os.path.isfile(playback_hook): | ||
| proc = subprocess.Popen( | ||
| playback_hook, stdin=subprocess.PIPE, | ||
| stdout=subprocess.PIPE, stderr=subprocess.STDOUT, |
There was a problem hiding this comment.
Didn't see that earlier - why do you capture stderr? I think it would be better if the caller would see any error messages on their command line.
There was a problem hiding this comment.
For now, I will merge this since I want to build on top of this. I think we should continue the discussion about what to to with stderr here, but that can be done in a later PR. For now, the hook script must not write anything to stderr in the case of success.
No description provided.