Issue 5212 - IRC hook should report bookmarked branches

Issue 5212 - IRC hook should report bookmarked branches

[Vasily Kuznetsov, 2017-05-05 09:23:08]

Hi Wladimir!

This looks good in general. I just have two nits below.

Cheers,
Vasily

https://codereview.adblockplus.org/29430555/diff/29430556/sitescripts/hg/bin/...
File sitescripts/hg/bin/irchook.py (right):

https://codereview.adblockplus.org/29430555/diff/29430556/sitescripts/hg/bin/...
sitescripts/hg/bin/irchook.py:10: branches = ctx.bookmarks()
I wonder if we could give a better name to this variable. `branches` seems a bit
confusing, especially on the next line where we're dealing with the actual
branch. Maybe we could just call it `bookmarks` (and consider that adding a
branch name to the list called `bookmarks` is a lesser confusion :). Or maybe
you can think of a better name -- nothing good comes to mind and I think it's
not really worth it to think more of this...

What do you think?

https://codereview.adblockplus.org/29430555/diff/29430556/sitescripts/hg/bin/...
sitescripts/hg/bin/irchook.py:11: if len(branches) == 0 or ctx.branch() !=
'default':
Since branches is a list we could just write `not branches` instead of
`len(branches) == 0`.

[Wladimir Palant, 2017-05-05 09:50:24]

https://codereview.adblockplus.org/29430555/diff/29430556/sitescripts/hg/bin/...
File sitescripts/hg/bin/irchook.py (right):

https://codereview.adblockplus.org/29430555/diff/29430556/sitescripts/hg/bin/...
sitescripts/hg/bin/irchook.py:10: branches = ctx.bookmarks()
On 2017/05/05 09:23:08, Vasily Kuznetsov wrote:
> I wonder if we could give a better name to this variable. `branches` seems a
bit
> confusing, especially on the next line where we're dealing with the actual
> branch. Maybe we could just call it `bookmarks` (and consider that adding a
> branch name to the list called `bookmarks` is a lesser confusion :). Or maybe
> you can think of a better name -- nothing good comes to mind and I think it's
> not really worth it to think more of this...
> 
> What do you think?

Let's go with commmit_identifiers.

https://codereview.adblockplus.org/29430555/diff/29430556/sitescripts/hg/bin/...
sitescripts/hg/bin/irchook.py:11: if len(branches) == 0 or ctx.branch() !=
'default':
On 2017/05/05 09:23:08, Vasily Kuznetsov wrote:
> Since branches is a list we could just write `not branches` instead of
> `len(branches) == 0`.

Done.

[Vasily Kuznetsov, 2017-05-05 09:57:33]

LGTM

Note: I haven't tested it but I assume you did.

https://codereview.adblockplus.org/29430555/diff/29430556/sitescripts/hg/bin/...
File sitescripts/hg/bin/irchook.py (right):

https://codereview.adblockplus.org/29430555/diff/29430556/sitescripts/hg/bin/...
sitescripts/hg/bin/irchook.py:10: branches = ctx.bookmarks()
On 2017/05/05 09:50:24, Wladimir Palant wrote:
> On 2017/05/05 09:23:08, Vasily Kuznetsov wrote:
> > I wonder if we could give a better name to this variable. `branches` seems a
> bit
> > confusing, especially on the next line where we're dealing with the actual
> > branch. Maybe we could just call it `bookmarks` (and consider that adding a
> > branch name to the list called `bookmarks` is a lesser confusion :). Or
maybe
> > you can think of a better name -- nothing good comes to mind and I think
it's
> > not really worth it to think more of this...
> > 
> > What do you think?
> 
> Let's go with commmit_identifiers.

I like this.

[Wladimir Palant, 2017-05-05 14:49:14]

On 2017/05/05 09:57:33, Vasily Kuznetsov wrote:
> Note: I haven't tested it but I assume you did.

I did, though in a somewhat limited way - this can only be tested properly in
the production environment.

[Vasily Kuznetsov, 2017-05-05 15:04:28]

On 2017/05/05 14:49:14, Wladimir Palant wrote:
> On 2017/05/05 09:57:33, Vasily Kuznetsov wrote:
> > Note: I haven't tested it but I assume you did.
> 
> I did, though in a somewhat limited way - this can only be tested properly in
> the production environment.

Well, as they say, real users are the best testers :D