Why is progress an object and not a function?

[barry-scott]

Having looked at the progress code I cannot see why I have to provided anything more
the the update() function. Requiring me to create a class seems to be asking me to do the work
of the library. Am I missing something?

The class has the internals of the parsing in it. Would I ever need to override this?

Why isn't details of the parse class hidden from the public API?

[Byron]

I see that the interface to the way progress is implemented seems over-engineered to you, which might be why this issue seems to be primarily about letting of some steam.

Now that you have gathered some experience with the code, maybe you have an idea on how to change it to make it more suitable for the common case you were seeing. Please feel free to submit a PR that would increase the usability to your liking.

[barry-scott]

I have patch for this in development. Need a bit of spare time to test and push.

[guyzmo]

Hi @barry-scott you should check the changes I've done to the API for this feature to actually work.

I did not change the implementation though, because (at least in my use case) an object was a good way to decouple UI work from git stuff:

https://github.com/guyzmo/git-repo/blob/devel/git_repo/services/service.py#L24-34

HTH

[barry-scott]

On 8 Jun 2016, at 19:13, guyzmo notifications@github.com wrote:

Hi @barry-scott https://github.com/barry-scott you should check the changes I've done to the API for this feature to actually work.

I did not change the implementation though, because (at least in my use case) an object was a good way to decouple UI work from git stuff:

https://github.com/guyzmo/git-repo/blob/devel/git_repo/services/service.py#L24-3 https://github.com/guyzmo/git-repo/blob/devel/git_repo/services/service.py#L24-34

I do not understand what you are getting at.

Here is a small example:

class GitStuff:
…

def doGitThing( self, progress ):

    self.index.pull( …, progress )

class UiStuff:
def init( self ):
self.git = GitStuff()
…

def updatreProgress( self, … );
    … update progress bar

def menuHandler( self ):
    self.git.doGitThing( self.updateProgress )

Given the above why did you need to change the GitPython code to provide anything more the
a function for progress updates?

Barry

HTH

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #436 (comment), or mute the thread https://github.com/notifications/unsubscribe/APwpu6QMsSEFO1EE_0BBZ3S5XW0B2RMiks5qJwYwgaJpZM4IjEqH.

[barry-scott]

On 8 Jun 2016, at 19:13, guyzmo notifications@github.com wrote:

Hi @barry-scott https://github.com/barry-scott you should check the changes I've done to the API for this feature to actually work.

I did not change the implementation though, because (at least in my use case) an object was a good way to decouple UI work from git stuff:

https://github.com/guyzmo/git-repo/blob/devel/git_repo/services/service.py#L24-34 https://github.com/guyzmo/git-repo/blob/devel/git_repo/services/service.py#L24-34

Oh now I see what you are doing… sorry I a bit slow on this.

This is what you should code.

class ProgressBar: # pragma: no cover
'''Nice looking progress bar for long running commands'''
def setup(self, repo_name):
self.bar = Bar(message='Pulling from {}'.format(repo_name), suffix='')

def update(self, op_code, cur_count, max_count=100, message=''):
    #log.info("{}, {}, {}, {}".format(op_code, cur_count, max_count, message))
    max_count = int(max_count or 100)
    if max_count != self.bar.max:
        self.bar.max = max_count
    self.bar.goto(int(cur_count))

somewhere else in your code you have lines that look like this:

bar = ProgressBar( repo_name )

git_index.pull( …, progress=bar.update )

At no time you you ever need to drag in the stderr parsing logic that is the reason for the RemoteProgress to exist.
Its a design bug in GitPython API to expose the parsing at all in my view.

I’m guessing you are not aware of the obj.func can be passed anywhere a func is required and python will invoke it
with obj as the self argument.

HTH

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #436 (comment), or mute the thread https://github.com/notifications/unsubscribe/APwpu6QMsSEFO1EE_0BBZ3S5XW0B2RMiks5qJwYwgaJpZM4IjEqH.

[guyzmo]

Given the above why did you need to change the GitPython code to provide anything more the a function for progress updates?

I was talking about the issue I opened I pushed a couple of weeks ago to get progress stuff to actually work correctly ☺

So what I meant when I said:

I did not change the implementation though, because (at least in my use case) an object was a good way to decouple UI work from git stuff:

is that I focused on getting the thing working, not on changing the suggested API.

I'm not saying it cannot be improved, though, just that it did not get in my way. Having the thing instantiated and with a method passed as a callback was short enough and readable with my use case. I agree that it can be improved further (and yes I'm aware of the many ways we can make it a one liner,

At no time you you ever need to drag in the stderr parsing logic that is the reason for the RemoteProgress to exist. Its a design bug in GitPython API to expose the parsing at all in my view.

First you're not "dragging the stderr parsing logic", merely a reference to it through the means of a base class.

The fact that it is a design bug is debatable.

I do not believe this is bad design to have just a RemoteProgress class that would be used as base interface for the user's object (with methods that raise NotImplementedError). Not really for typing reasons (because 🦆), but for documentation and helping use the tool right, and because just on the first line of the class you understand to what this class belongs to:

class ProgressBar(RemoteProgress):
    …

Then, importing a class with some logic to be extended with a child class, as it's currently designed, is not a bad one either, for the same reasons as having an interface, but also because it makes it easier to override how parsing is done in case someone wants to do things slightly differently without having to monkey patch the module.

For the note, that's the way the threading.Thread module has been designed in the python standard library.

I’m guessing you are not aware of the obj.func can be passed anywhere a func is required and python will invoke it with obj as the self argument.

I do know about that, and there are many other ways to work around the use of an object in this situation… Timtoady, but there should be one — and preferably only one ­— obvious way to do it.

---

Though thinking about it, maybe could we have it both ways. Sometimes we might want a simple function to handle progress, sometimes we want something more heavy that could even need some under-the-hood tweaks. So what could be done would be to have the pb argument of fetching methods accept both an object or a function!

Then if pb is a function, it instanciates the default RemoteProgress that would call pb within the update() method… And if it is an object it looks up the init() and update() methods and call them!

So then we could have:

git_index.pull( …, progress=lambda op_code, cur_count, max_count=100, message='': …)

or

pb = ProgressBar()
git_index.pull(…, progress=pb) 

the way I did in my code.

[guyzmo]

N.B.: the 🦆 emoji is a duck (you'll only see it if you're using an unicode 9 font).

[guyzmo]

ahah, in the end, what I just said is what you've done ☺ so we do agree!

I missed the fact that there's been a patch along with this issue, I thought it's been closed because I worked on a separate thing related to progress.

👍

[barry-scott]

First thank you for spotting and fixing the progress buffering issue.

I think you believe that its a feature that you can derive a class from RemoteProgress to use
in the progress API. I believe that is a API design error. I do not think we agree on this.

As you noticed the patch that implemented this change has full backwards compatibility.
If it was not for backwards compatibility there is no need for the RemoteProgress (with parser)
to exists as far as the caller of the API is concerned.

Your example does not need to derive from RemoteProgress. Call GitPython like this:

pb = ProgressBar()
git_index.pull(…, progress=pb.update) 

The only place where RemoteProgress is needs to be accessed is to get at the state constants:
BEGIN, END, COUNTING, COMPRESSING, WRITING, RECEIVING, RESOLVING, FINDING_SOURCES, CHECKING_OUT