1228358 - Further improvements of pushlog_client

Status: RESOLVED FIXED

(Developer Services :: Mercurial: Pushlog, defect)

Description

[MikeLing]

After talk with :AutomatedTester on irc,  there are still a lot of places we could been improved for pushlog client. one of them which can be sure is we could change 'query_revisions_range_from_revision_before_and_after' function name to reasonable length. And the other one is we may need to make pushlog_client return a python object which can represent a push.

So I would like to ask his opinion in here and anyone who would like to give suggestion for pushlog_client can leave comment in here :)

Comment 1

[David Burns :automatedtester]

I think that there needs to be a python object, that allows easier documentation of what is returned to users from using the API. See https://github.com/AutomatedTester/Bugsy/blob/master/bugsy/bug.py#L14 as an example.

I also think the API names need to be revisited. As mentioned in comment 0, that call is hard to mentally parse to understand what it does. It feels like there are methods that could be combined and default params help guide someone using it.

Comment 2

[MikeLing]

(In reply to David Burns :automatedtester from comment #1)
Ok, I will make it happened within today :)

Comment 3

[Armen [:armenzg]]

Side note: we should look at what there is in vct wrt to classes.
For instance a PushInfo class:
https://hg.mozilla.org/hgcustom/version-control-tools/file/default/pylib/mozautomation/mozautomation/repository.py#l226

I think coming up with a draft with gps which we can review would help us do things right from the beginning.

gps: do you have any classes which represent pushes?

Comment 4

[Gregory Szorc [:gps]]

The mozautomation package is more or less a catch all of random utilities and many of them are half baked and only implement features necessary to support something else. The pushlog-related code in there should probably be deleted and consumers should be switched to the new pushlog client package you just released.

The pushlog classes you found are currently the best there are. You are probably best reinventing the wheel.

Comment 5

[MikeLing]

Attachment: in progress PR

Hi Armen and David, here is the progress I had achieved for now. We return a list of push object rather than return revision list directly now. Actually, I always wondering if I'm on the right direction for this issue. Could you give some advice about it? Thank you!

BTW, I think those function name may changed from eg. query_revision_ to query_*push*_by_revision, which will make function name become more longer ;)

Comment 6

[David Burns :automatedtester]

commented directly on the PR

Comment 7

[Armen [:armenzg]]

What David mentions in the PR is in the right direction

Comment 8

[MikeLing]

Attachment: PR for bug 1228358

Maybe this PR is good to go? Let's just forget the test part, we will need rewrite the whole tests in bug 1227531 :)

Comment 9

[David Burns :automatedtester]

Comment on attachment 8696582 [details] [review]
PR for bug 1228358

This is significantly better from what it was. The API now allows more people to use python idioms to build out what they might need. 

For me this is good enough to go (after the nits have been looked at).

Comment 10

[Armen [:armenzg]]

Comment on attachment 8696582 [details] [review]
PR for bug 1228358

Given feedback in the PR.
A bit more work is required.

Comment 11

[Armen [:armenzg]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/f879e533347324721f1ac31c73d541c188deee74
Bug 1228358 - Pushlog client to manage objects and minor refactoring. r=armenzg

Comment 12

[Armen [:armenzg]]

Comment on attachment 8696582 [details] [review]
PR for bug 1228358

I've landed this! Thanks MikeLing :)

I replaced though **kwargs for push_info. If we ever really needed we can add it back but for now I decided we really don't need it.

I also made the mistake of not specifying -u "MikeLing" in the commit message. My apologies about this.

I also released a 0.2.0 version. You can now make changes to mozci to use this package :D

Comment 13

[MikeLing]

(In reply to Armen Zambrano Gasparnian [:armenzg] from comment #12)
> Comment on attachment 8696582 [details] [review]
> PR for bug 1228358
> 
> I've landed this! Thanks MikeLing :)
It's my pleasure :) BTW, could we close that PR if we had landed it?

> I also made the mistake of not specifying -u "MikeLing" in the commit
> message. My apologies about this.

np, I had learn a ton of things from this issue. So I'm ok with it :)

> I also released a 0.2.0 version. You can now make changes to mozci to use
> this package :D

Great! I will start my work right now!

Comment 14

[Armen [:armenzg]]

I can't close the PR. Only the person who started it can (plus admins).

Could you check if you can close it?

Comment 15

[MikeLing]

Yes, I can close it. But I add some more code in that branch in (https://github.com/mozilla/version-control-tools/pull/2/files), maybe we could close it after merge that :)

Comment 16

[Armen [:armenzg]]

I saw that you pushed more code into the PR.
In general, start a new PR if there is some follow up work.

I will close this bug for now. We can open new ones.

Comment 17

[Armen [:armenzg]]

More changes from MikeLing:
https://github.com/mozilla/version-control-tools/pull/3

Comment 18

[Armen [:armenzg]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/8a0940aacdb60fadf27d29c81101423fd19fcdfe
Bug 1228358 - Add query_pushes_by_specified_revision_range() to pushlog client. r=armenzg

Comment 19

[Armen [:armenzg]]

Attachment: Add query_pushes_by_specified_revision_range() to pushlog client. r=armenzg

Comment 20

[Armen [:armenzg]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/a6cba09c935e694588bb619e40c73b69bbd16073
Bug 1228358 - Release version pushlog_client 0.3.0

Comment 21

[Armen [:armenzg]]

My apologies for the delayed release :/