Skip to content

Provide complete() hook on RemoteProgress #1018

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

Open
pzelnip opened this issue Jun 7, 2020 · 3 comments
Open

Provide complete() hook on RemoteProgress #1018

pzelnip opened this issue Jun 7, 2020 · 3 comments

Comments

@pzelnip
Copy link

pzelnip commented Jun 7, 2020

It'd be nice to have a method or hook where one could define clean-up actions on a RemoteProgress indicator. The update() method allows one to update progress info, but there's no way to know when a RemoteProgress indicator completes. I'm envisioning a complete() method on a RemoteProgress instance that would be implemented by childclasses that GitPython would call once an action that takes a progress indicator completes.

Think for example a progress indicator that after the operation has completed spits out a "progress complete" message. Right now I do that by passing the RemoteProgress instance to clone_from and then after the clone_from call comes back I do a separate update to indicate operation complete. This is fine for basic sequential (ie multiple line) output to stdout, but say if you want to use a library that does progress bars (ex: Enlighten or alive-progress) that updates the same line in the terminal window, suddenly it gets a bit awkward as "ownership" of that line on screen is shared between the caller of clone_from and the RemoteProgress instance.

@Byron
Copy link
Member

Byron commented Jun 8, 2020

Thanks for reporting!
I agree, knowing when the parsing is done would be useful to have and it seems like an oversight not to have it.

Even without changing the current API, it should be possible to implement by making additional calls here.

As the handler is merely a function, maybe a special 'marker' value can be passed instead of line that signals the end of the stream.

@pzelnip
Copy link
Author

pzelnip commented Jun 14, 2020

I think I get what you're getting at, maybe I could confirm:

  • modify handle_prcess_output() in the pump_stream helper to emit a special marker value when the for line in stream loop is complete
  • then in RemoteProgress._parse_progress_line() check if the incoming line is that special value. If it is, then call RemoteProgress.complete() (which would be a new method like update() that implementing classes would override to do cleanup/completion logic)

Is that what you were thinking? I think that's a bit more granular than I had in mind, I'm not interested in hooking into say when a "counting objects" or "compressing objects" operation is complete, but rather the higher level clone operation is complete. I could see both being useful though, for example: you spin off a separate progress bar for each individual sub operation of the clone, which would have to be cleaned up/stopped when each sub-operation is complete, but then you also want to have an overall progress indicator for the clone operation as a whole, and you'd have to clean up/stop that when the overall clone operation is complete.

@Byron
Copy link
Member

Byron commented Jun 15, 2020

I believe we had the same in mind, as what I envisioned wouldn't be granular at all. Effectively, another 'special/marked' handler call would be inserted here, which can be transformed into a new complete() call to not confuse anyones code out there.

That should also be quite straightforward to implement, in case you would like to give it a try.

Inarguably, it's a bit odd to have a function, that in fact always delegates to a class which does the parsing, so instead of just calling complete() on an instance of that class, one now has to provide special input to that handler in order to make the right call.

The reason that was done is to remove the complexity that stems from classes, and make it very easy to just output every line one by one. These handlers would now see such a marker, which might subtly break expectations people have in the handler. And I just realized that it's one handler for each kind of output, one for stderr, and one for stdout. Thus the complete call can easily be emitted twice.

I believe it would be best to have not change that handler at all, but instead emit a complete call where it is known that we are actually having a class instance that supports it.

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

No branches or pull requests

2 participants