330506 - [MODULE] Add a module to nit-review patches using jst-review

Status: RESOLVED FIXED

(Webtools Graveyard :: Mozbot, enhancement, P3)

Description

[:Gijs (he/him)]

Several people wondered whether we could have functionality in mozbot/firebot to nit-review new patches. Of course, a review from just a bot is not enough, but it can give helpful indications on how to improve at times.

Allan Beaufour wrote/fixed/changed the jst-review script that does this, and I wrote a mozbot module that uses it to tell you about what's wrong with your patch.

I will attach the module in a bit. I'm not sure whether I'm "allowed" to add another library to the list of requirements to use mozbot... That'll be for the reviewers to decide.

Also, please note this is the first substantial bit of perl I've ever written. I realize that it's probably not too great yet, but I'm willing to fix whatever's wrong with it (within reason, of course).

Comment 1

[:Gijs (he/him)]

Attachment: Review module

I'm assuming Hixie is the right person to ask for review on this. If someone else spots something, please feel free to do so. If anyone's entitled to review it, apart from Hixie, and they have time to do so, feel free to review it in that case. I hear Hixie's busy ;-).

Comment 2

[:Gijs (he/him)]

Comment on attachment 215062 [details]
Review module

Fixing mimetype, I hope...

Comment 3

[:Gijs (he/him)]

Merm. I just realized that this still has
$self->debug($review_line);
which should obviously be removed. Sorry. Sparing you all the bugmail of a new attachment + obsoleting the old one and retracting the review request. Or so I hope.

Comment 4

[Hixie (not reading bugmail)]

This is very Mozilla-specific, so probably shouldn't be a module that is shipped with mozbot (assuming I ever get around to shipping mozbot!). Can you run it on one of the bots for a while and see how it works?

Comment 5

[:Gijs (he/him)]

OK. For the record, firebot on moznet runs this module now (Thanks Wolf!).

Comment 6

[Robert Kaiser]

It would be nice to have that module in CVS though, so that one could easily get the most current version of it

Comment 7

[Hixie (not reading bugmail)]

Maybe a moz-specific CVS directory?

Comment 8

[:Gijs (he/him)]

Attachment: Move Bugzilla, Sheriff and Tinderbox modules to separate directory

This patch moves the Bugzilla, Sheriff and Tinderbox modules to a subdirectory of BotModules named 'MozSpecific' (it also moves the BugzillaMailHandler stuff)

In order for things to continue to work after this move, I've also changed two things:

- Made LoadModule in mozbot.pl check for modules in aforementioned subdirectory if they do not exist in the BotModules directory
- Moved the location of the bugmail.log file to the new subdirectory. As far as I can tell, the only thing that needed to be adjusted for that was the BugzillaMailHandler path, as the Bugmail module just expects the logfile to be in the same dir it lives in.

Hixie, would this do for the "moz-specific CVS directory" that you wanted?
(Note that I have not yet added the nit-review module at all, I presume you would want to do a more careful (re-)review of it before it actually goes into CVS)

Comment 9

[Reed Loden [:reed]]

Comment on attachment 278193 [details] [diff] [review]
Move Bugzilla, Sheriff and Tinderbox modules to separate directory

If you're going to do this (though I don't see why it is needed, as mozbot is dead), please do a CVS copy instead of a move this way, or else you're going to lose all the CVS history, which would be really bad.

Comment 10

[:Gijs (he/him)]

Reed: fair point about the CVS copy, but that's rather hard to diff.

Furthermore, how "dead" is mozbot? Is there a bug filed to get a new owner for it if Ian is not doing anything to it anymore? Also, you yourself seem to have checked something into it at the beginning of this year, so apparently it's not *that* dead.

Comment 11

[Wolf [:wolf]]

'Dead' is a relative term for a project thats not very active to start with. It seems to come from bug 365975.

Though, the Bugzilla module is certainly not Mozilla specific. Tinderbox is, as-is sheriff. though, I don't think moving them is a good idea, as it makes it more difficult to maintain a mozbot (i.e. afaik, mozbot won't load a module in a subdirectory of modules, so you'd have to move it to the main directory to load it, and make cvs up'ing the bot for patches harder.)

Comment 12

[:Gijs (he/him)]

(In reply to comment #11)
> 'Dead' is a relative term for a project thats not very active to start with. It
> seems to come from bug 365975.
> 
> Though, the Bugzilla module is certainly not Mozilla specific. Tinderbox is,
> as-is sheriff. though, I don't think moving them is a good idea, as it makes it
> more difficult to maintain a mozbot (i.e. afaik, mozbot won't load a module in
> a subdirectory of modules, so you'd have to move it to the main directory to
> load it, and make cvs up'ing the bot for patches harder.)
> 
Which is why I hacked up mozbot.pl to look into the newly created directory as well. ;-) (I would have made it general, but this is sort of hard given the way mozbot looks for modules)

Comment 13

[Wolf [:wolf]]

Oh. Silly me, I should've guessed. :-) Tinderbox probably could/should be made not bound to be mozilla, other organizations do use Tinderbox, and there's an outstanding bug on adding support for customizing the URL and adding Tinderbox2 and 3 support.

You probably could move sheriff into a mozilla specific directory though, but its a dead module, for the most part. Even Tinderbox1's default configuration breaks it.

As for the patch to mozbot.pl and landing this module in a mozilla specific directory, I don't see why that'd be a problem. :-) but reviews for mozbot are eternal, given Ian's business and lack of time for mozbot.

Comment 14

[Wolf [:wolf]]

Resetting to new default owner.

Comment 15

[Wolf [:wolf]]

Comment on attachment 215062 [details]
Review module

marking review so this is more obviously on my radar..

Comment 16

[Wolf [:wolf]]

Comment on attachment 215062 [details]
Review module

It can join the ranks of mozbot.

Comment 17

[Wolf [:wolf]]

Checking in Review.bm;
/cvsroot/mozilla/webtools/mozbot/BotModules/Review.bm,v  <--  Review.bm
initial revision: 1.1
done

Comment 18

[:Gijs (he/him)]

(In reply to comment #16)
> (From update of attachment 215062 [details])
> It can join the ranks of mozbot.

Awesome, thanks! :-)