@mprahl @jkaluza @lucarval Could you please review as well?

What's the difference between done_percent vs finish_percent ?

Is it really necessary to convert "complete" to "done"? I think this adds complexity, it's much simpler to simply use the build state as is. Maybe just use str.lower()?

Wouldn't omitting these cause more confusion? For instance, adding up all the states may not add up to total.

Consider using operator's itemgetter:

from operator import itemgetter
sorted(injected_task_infos, key=itemgetter('state'))
groupby(sorted_tasks_info, itemgetter('state'))

I think this can be rewritten without lambda:

builds_info = pool.map(self.module_get_build, build_ids)

@lucarval

Is it really necessary to convert "complete" to "done"? I think this adds complexity, it's much simpler to simply use the build state as is. Maybe just use str.lower()?

I just thought to display something like what Koji watch command output, for example, Components: 1 done, 0 failed., and format string could also include a same format argument, for example the code p(' Components: %(done)s done, %(failed)s failed' % stats).

What's the difference between done_percent vs finish_percent ?

Good catch. It should be finish_percent.

Wouldn't omitting these cause more confusion? For instance, adding up all the states may not add up to total.

Watch only shows the number of builds in done, failed and building. State done and failed are treated as finish states, the finish_percent is calculated based on them.

Consider using operator's itemgetter:

from operator import itemgetter
sorted(injected_task_infos, key=itemgetter('state'))
groupby(sorted_tasks_info, itemgetter('state'))

This is really a better way. Thanks!

I think this can be rewritten without lambda:

builds_info = pool.map(self.module_get_build, build_ids)

Indeed. :thumbsup: :)

@lucarval Can you review again? All your comments are addressed.

So this will require a Keyboard Interrupt to stop?

Can we also stop watching if all the builds are completed?

Nitpick: Instead of nesting the code in the if-block, the code can be made more readable by inverting the conditional:

if bi['state_name'] == 'init':
  continue
# Process non-init states outside of if-block.

Optional: It's actually slower to use dict() instead of {}

https://doughellmann.com/blog/2012/11/12/the-performance-impact-of-using-dict-instead-of-in-cpython-2-7-2/

+1. It looks like the method that calls this actually breaks when the module build is done, but it'd be cleaner if this method quit the loop when the module build completed.

Optional: I think it'd be more readable if this was called module_builds

Optional: I think it'd be more readable if bi was called module_build

+1

Optional: I think it'd be easier to read if injected_task_infos were to be called formatted_tasks.

It'd be easier to read if you didn't use dict but instead used the {} syntax.

Optional: You could probably just reassign injected_task_infos here since all you're doing is sorting it.

This is quite short. How about at least 5 seconds between iterations of the loop?

Optional: The key completion_percentage is likely what you were going for

Instead of (stats['done'] + stats['failed']) * 1.0, how about float(stats['done'] + stats['failed'])?

This isn't a blocker but I'm not a fan of this since it just saves 4 characters and makes it harder to read.

Could you rename builds_info to module_builds?

Could you rename bi to module_build?

How about the following?

if (state_name != 'build' and last_builds.get(module_build_id, {}).get('state_name') == state_name):

Very nice

This docstring should be Watch MBS builds from the command-line.

I'd prefer if we displayed name:stream:version:context instead of name-stream.

@cqi this looks great. Thanks for the demo!

I left some comments. Please let me know if you have any questions.

Optional: It's actually slower to use dict() instead of {}

Done.

Optional: The key completion_percentage is likely what you were going for

Done.

Instead of (stats['done'] + stats['failed']) * 1.0, how about float(stats['done'] + stats['failed'])?

Done.

So this will require a Keyboard Interrupt to stop?

No. get_watched_module_builds is a generator, Python will handle StopIterator and while-loop terminates when break is executed in module_watch_build.

Catching Keyboard Interrupt is just for printing some information for user what could do to continue watching builds.

Can we also stop watching if all the builds are completed?

@lucarval What do you mean here?

Optional: I think it'd be more readable if this was called module_builds
Optional: I think it'd be more readable if bi was called module_build

Done.

Nitpick: Instead of nesting the code in the if-block, the code can be made more readable by inverting the conditional:
if bi['state_name'] == 'init':
continue
# Process non-init states outside of if-block.

Whatever the state is, module_build['link'] must be set, and only calculate statistics for non-init state. So, we cannot simply skip init like that.

Optional: I think it'd be easier to read if injected_task_infos were to be called formatted_tasks.

Done.

Optional: You could probably just reassign injected_task_infos here since all you're doing is sorting it.

Done.

It'd be easier to read if you didn't use dict but instead used the {} syntax.

@mprahl I guess you mean the syntax like this, right?

bi['tasks']['rpms'] = {
    task_state: sorted(data, key=itemgetter('package_name'))
    for task_state, data in groupby(sorted_tasks_info, itemgetter('state'))
}

Python 2.6 has to be considered for rpkg. That syntax is not supported by py2.6.

This is quite short. How about at least 5 seconds between iterations of the loop?

Sure. Updated.

This isn't a blocker but I'm not a fan of this since it just saves 4 characters and makes it harder to read.

Understood :) Since there are too many strings output to stdout by calling print function, I just wanted to keep the function name inside each line of those print lines shortest enough in order to easily focus eyes on the string content output as much as possible and ignore the function name from eyes.

I'd prefer if we displayed name:stream:version:context instead of name-stream.

Done.

Could you rename builds_info to module_builds?
Could you rename bi to module_build?

Done.

How about the following?
if (state_name != 'build' and last_builds.get(module_build_id, {}).get('state_name') == state_name):

Done.

This docstring should be Watch MBS builds from the command-line.

Done.

Can we also stop watching if all the builds are completed?

@lucarval What do you mean here?

I misread the code and thought the watch would keep going when all builds had finished. Discard this comment.

Nitpick: Instead of nesting the code in the if-block, the code can be made more readable by inverting the conditional:
if bi['state_name'] == 'init':
continue

Process non-init states outside of if-block.

Whatever the state is, module_build['link'] must be set, and only calculate statistics for non-init state. So, we cannot simply skip init like that.

Couldn't we move setting module_build['link'] up in the for-loop?

@cqi I agree with @lucarval's comment but it's not a blocker from my point of view. :thumbsup: from me.

Couldn't we move setting module_build['link'] up in the for-loop?

@lucarval @mprahl :thumbsup: Right :) Patch is updated. Thanks a lot for your comments.

Can this logic be moved to get_watched_module_builds? That way the generator just stops once all the builds are finished. That seems more Pythonic to me.

@mprahl Fixed.

Nice, :thumbsup:

@lsedlar @onosek Ping for review and possibly merge. :)

Looks good to me. I didn't test it though.

+1, I have tried running that and it works as expected for me.

I have few suggestions for changes, but I don't want @cqi to prolong this review. I will submit separate PR later. They are mainly cosmetic changes to the output.

It looks also good for me. Merging.