The personal blog of a Toronto based software developer, musician, sound designer, and theatre enthusiast.
Archive for the ‘Extensions’ Category.
I know, I know. I left you all hanging at the edge of your seat with my last blog post, and I still haven’t posted my idea for recognizing good code review.
I’m bogged down with school work, and I’m aiming to have the first draft of my research paper done next week. So that’s taking 100% of my resources.
Just be patient. I’ll post my idea soon.
While the benefits of code review are proven, documented, numerous and awesome, it doesn’t change the fact that most people, in general, don’t like doing it.
I guess code review just isn’t really all that fun.
So a few months ago, I broadcast the idea of turning code review into a game. It was my way of trying to mix things up – “let’s add points, and have reviewers/developers competing to be the best participant in the code review process”.
Well, if there’s one thing that my supervisor Greg has taught me, it’s how I shouldn’t rush headlong into something before all of the facts are in. So before I decide to do something like game-ifize code review, I should take a look at some prior work in the area…
Enter this guy: Sebastian Deterding.
In particular, check out the following slide-show. Flip through it if you have the time. If you don’t have the time, scroll down, where I get to the salient point with respect to game-ificating code review.
Here’s the slide-show. Be sure to read the narrative at the bottom.
Sebastian seems to be saying that adding points to apps and trying to incite competition does not make something a game. If it did, then this should be countless hours of fun.
Without play, there is no game. Points do not equal a game. It’s not nearly that simple.
I’m going to divert for a second here.
Last week, a company set themselves up a couple of booths in the lobby of the Bahen Center where I work. They were there to recruit university students to work for their company – either as interns, or full-timers.
They were also handing out free pizza and pop.
Needless to say, I wanted a few slices – but I figured it would be polite if I engaged them in conversation before waltzing off with some of the free food and drink they’d brought.
So I sparked up a conversation with one of the recruiters, and he told me about the company. I’m going to call this recruiter Vlad.
I ended up gently steering the conversation towards code review, and I asked my inevitable question:
“So, do you guys do code review?”
I felt like a dentist asking a patient if he’s been flossing. Vlad waffled a bit, but the general impression was:
“Not as much as we should. We don’t have a prescribed workflow. It’d be hard to persuade all of the teams to do it.”
And then we started talking about code review in general. It turns out that Vlad had worked in a few companies where they’d done code review, and he always felt a little short changed. He said something along the lines of:
“I never felt compelled to do reviews. They just sort of happened…and I did it, and it felt like…unrecognized effort. I mean, what’s the incentive? Do you know what I mean? There’s incentive for the software, but I’m talking incentive for me. And some people did really lousy reviews…but my reviews were treated the same as theirs. I didn’t get recognized, and didn’t get rewarded if I did a good review. So it was hard for me to do them. I want to be recognized for my good reviews, for my good contributions.”
I wish I’d had a tape-recorder running so I could have gotten Vlad’s exact words. But that’s what I remember him saying.
Maybe instead of trying to game-ulize code review, I can instead hear what Vlad is saying and work off of that.
With the code review that Vlad participated in, all of the feedback went to the code author, and none went to the reviewers. And the reviewers are the ones who are doing all of the heavy lifting! As a reviewer, Vlad also wants feedback, and recognition for code review done well.
There’s a company in Toronto that specializes in feedback like this. They’re one of the major players in the Toronto start-up scene, and have built a pretty sweet suite of tools to facilitate quick and easy feedback/recognition.
The company is called Rypple. And maybe that’s the name of the application, too. (checks website) Yeah, it’s both.
So Rypple has this feature called Kudos that let’s people publicly acknowledge the good work of their team.
Normally, I don’t pimp companies. And it upsets me when people comment on my blog, and their sub-text is to try to sell their product or service. However, I think this video is relevant, so I’m posting their demo video so you can see how Kudos work:
Click here if you can’t see the video.
So Rypple’s idea is to have a feed that the team subscribes to, and publicly display things like Kudos. The badges for the Kudos are also limited in how many you can give per week, so they’re a valuable commodity that can’t just be handed out all over the place. Cool idea.
So there’s one approach – use a service like Rypple to give your reviewers better feedback and recognition.
Or maybe we could build an extension for Review Board that does something similar, and more oriented around code review.
It’s not oriented like a game, like I had originally envisioned. But somehow, I think this idea has more meaning and traction than just “adding points”.
More on this idea in a few days. But please, comment if you have any thoughts or ideas to add.
In my last post, I talked about an extension for Review Board that would allow users to register “defects”, “TODOs” or “problems” with code that’s up for review.
After chatting with the lead RB devs for a bit, we’ve decided to scrap the extension.
[audible gasp, booing, hissing]
Instead, we’re just going to put it in the core of Review Board.
[thundering applause]
Why is this useful? I’ve got a few reasons for you:
However, since we’re adding this to the core of Review Board, we have to keep it simple. One of Review Board’s biggest strengths is in its total lack of clutter. No bells. No whistles. Just the things you need to get the job done. Let the extensions bring the bells and whistles.
So that means creating a bare-bones defect-tracking mechanism and UI, and leaving it open for extension. Because who knows, maybe there are some people out there who want to customize what kind of defects they’re filing.
I’ve come up with a design that I think is pretty simple and clean. And it doesn’t rock the boat – if you’re not interested in filing defects, your Review Board experience stays the same.
I propose adding a simple checkbox to the comment dialog to indicate that this comment files a defect, like so:
No bells. No whistles. Just a simple little checkbox.
While I’m in there, I’ll try to toss in some hooks so that extension developers can add more fields – for example, the classification or the priority of the defect. By default, however, it’s just a bare-bones little checkbox.
So far, so good. You’ve filed a defect. Maybe this is how it’ll look like in the in-line comment viewer:
A defect has been reported!
A reviewer can file defects reports, and the reviewee is able to act on them.
Lets say I’m the reviewee. I’ve just gotten a review, and I’ve got my editor / IDE with my patch waiting in the background. I see a few defect reports have been filed. For the ones I completely agree with, I fix them in my editor, and then go back to Review Board and mark them as Fixed.
All fixed!
It’s also possible that I might not agree with one or more of the defect reports. In this case, I’ll reply to the comment to argue my case. I might also mark the defect report as Pass, which means, “I’ve seen it, but I think I’ll pass on that”.
I think I'll pass on that, thanks.
These comments and defect reports are also visible in the review request details page:
A defect has been filed.
All fixed up.
It's all good - just pass this defect report.
What do you think? Am I on the right track? Am I missing a case? Does “pass” make sense? Will this be useful? I’d love to hear your thoughts.
I just spent the long weekend in Ottawa and Québec City with my parents and my girlfriend Em.
During the long drive back to Toronto from Québec City, I had plenty of time to think about my GSoC project, and where I want to go with it once GSoC is done.
Here’s what I came up with.
I think it’s a safe assumption that my reviewing-time extension isn’t going to be the only one to generate useful statistical data.
So why not give extension developers an easy mechanism to display statistical data for their extension?
First, I’m going to extract the reviewing-time recording portion of the extension. Then, RB-Stats (or whatever I end up calling it), will introduce it’s own set of hooks for other extensions to register with. This way, if users want some stats, there will be one place to go to get them. And if an extension developer wants to make some statistics available, a lot of the hard work will already be done for them.
And if an extension has the capability of combining its data with another extensions data to create a new statistic, we’ll let RB-Stats manage all of that business.
The reviewing-time feature of RB-Stats will become an extension on its own, and register its data with RB-Stats. Once RB-Stats and Stopwatch are done, we should be feature equivalent with my demo.
I kind of breezed past this in my demo, but I’m interested in displaying “review karma”. Review karma is the reviews/review-requests ratio.
But I’m not sure karma is the right word. It suggests that a low ratio (many review requests, few reviews) is a bad thing. I’m not so sure that’s true.
Still, I wonder what the impact will be to display review karma? Not just in the RB-Stats statistics view, but next to user names? Will there be an impact on review activity when we display this “reputation” value?
This is a big one.
Most code review tools allow reviewers to register “defects”, “todos” or “problems” with the code up for review. This makes it easier for reviewees to keep track of things to fix, and things that have already been taken care of. It’s also useful in that it helps generate interesting statistics like defect density and defect detection rate (assuming Stopwatch is installed and enabled).
I’m going to tackle this extension as soon as RB-Stats, Stopwatch and Karma are done. At this point, I’m quite confident that the current extension framework can more or less handle this.
Got any more ideas for me? Or maybe an extension wish-list? Let me know.
If I’ve learned anything from my supervisor, it’s to demo. Demo often. Step out of the lab and introduce what you’ve been working on to the world. Hit the pavement and show, rather than tell.
So here’s a video of me demoing my statistics extension for Review Board. It’s still in the early phases, but a lot of the groundwork has been taken care of.
And sorry for the video quality. Desktop capture on Ubuntu turned out to be surprisingly difficult for my laptop, and that’s the best I could do.
So, without further ado, here’s my demo (click here if you can’t see it):
Not bad! And I haven’t even reached the midterm of GSoC yet. Still plenty of time to enhance, document, test, and polish.
If you have any questions or comments, I’d love to hear them.
From the very beginning, my GSoC project has been mainly focused towards one primary goal: I want to build an extension for Review Board that will allow me to collect information about how long reviewers actually spend reviewing code.
That’s easier said than done. When I started, the Review Board extension framework wasn’t really in a state to allow such an extension to exist.
So I’ve been tooling around in the Review Board code for the past 2 months, preparing the framework, and getting it ready to handle my extension.
And last night, it started to work. I can now give rough estimates on how long a reviewer has spent reviewing code.
My extension adds a new table to the database which stores “reviewing sessions”. Each reviewing session is associated to a particular review request and user, and also has a field to store the number of seconds that a user has spent in review.
I’ve created a TemplateHook that allows me to inject Javascript into key areas of Review Board (in particular, the diff viewer, and the screenshot viewer). The Javascript does the following: every 10 seconds, we check to see if the mouse has moved on the body of the HTML document. If it has, we send an “activity” notification to the server.
The server receives this activity notification through the Web API, and checks to see if the time lapsed since the last session update was greater than 10 seconds. If it is, we increment the working session by 10 seconds and return a 200 HTTP code. If it isn’t, we don’t change anything and return a 304 HTTP code.
Next, my extension waits for a user to publish a review. When it notices that a review is being published, it finds the working session for that user and review request, and then attaches it to the published review. If the user then starts looking at the diff or screenshots again, a new working session is created.
The result? A pretty decent estimate of how long a user has spent reviewing the code. No time gets recorded if the user gets up and has a sandwich. No time gets recorded if the user is on another tab reading Reddit.
Not bad. For a first draft, anyhow.
I think I’m going to try to chart the data somehow, so that users can track their inspection rates. I’ll let you know how that goes.
I can’t honestly say I’ve been doing much test-driven development on Review Board. Django is a really cool web-framework, but I miss all of the nice testing tools from the Rails ecosystem.
Anyhow, today, I decided to run the tests, and BAM – Segmentation Fault:
Testing Perforce binary diff parsing … SKIP: perforce/p4python is not installed
Testing PerforceTool.get_changeset … SKIP: perforce/p4python is not installed
Testing Perforce empty and normal diff parsing … SKIP: perforce/p4python is not installed
Testing Perforce empty diff parsing … SKIP: perforce/p4python is not installed
Testing PerforceTool.get_file … SKIP: perforce/p4python is not installed
Testing parsing SVN diff with binary file … ok
Testing SVNTool.get_file … Segmentation Fault
Yikes. Getting a segfault in Python is unusual, and a little jarring. However, it did let me narrow down the problem a bit: it must have to do with the pysvn bindings that Review Board uses to talk to Subversion, because those are written in C++ (which can certainly segfault).
The machine I was using had Ubuntu Hardy on it, and the Hardy packages have pysvn 1.5.2-1. Taking a look at the pysvn homepage, the most recent version is actually 1.7.2.
So, I uninstalled pysvn, downloaded the source for 1.7.2, compiled it, and installed it manually.
Segfault gone. Tests pass. Awesome sauce.
Every now and then, the computer gods smile and give me a freebie.
I’ve been worrying my mind out over a few problems / obstacles for my Review Board extensions GSoC project. In particular, I’ve been worrying about dealing with extension dependencies, conflicts, and installation.
I racked my brain. I came up with scenarios. I drew lots of big scary diagrams on a wipe board.
And then light dawned.
Enter Setuptools and Python Eggs.
All of those things I was worried about having to build and account for? When using Python Eggs, It’s all built in. Dependencies? Taken care of. Conflicts? Don’t worry about it. Installation? That’s what Setuptools and Python Eggs were built for!
In fact, it even looks like Setuptools was designed with extensible applications in mind.
Here’s the setup.py file for the rb-reports extension in the rb-extensions-pack on Github:
from setuptools import setup, find_packages
PACKAGE="RB-Reports"
VERSION="0.1"
setup(
name=PACKAGE,
version=VERSION,
description="""Reports extension for Review Board""",
author="Christian Hammond",
packages=["rbreports"],
entry_points={
'reviewboard.extensions':
'%s = rbreports.extension:ReportsExtension' % PACKAGE,
},
package_data={
'rbreports': [
'htdocs/css/*.css',
'htdocs/js/*.js',
'templates/rbreports/*.html',
'templates/rbreports/*.txt',
],
}
)
Pay particular attention to the “entry_points” parameter. What this is doing, is registering rbreports.extension:ReportsExtension to the entry point “reviewboard.extensions”.
“Hold up!”, I hear you asking. “What’s an entry point?”
An entry point is a unique identifier associated with an application that can accept extensions.
The unique identifier for Review Board extensions is “reviewboard.extensions”.
This is the first handshake, more or less, between Review Board and any extensions: in order for Review Board to “see” the extension, the extension must register an entry point at “reviewboard.extensions”.
This blog post shows how extensions can be found and loaded up.
I remember also being worried about how to create tables in Django for extension models. I thought “holy smokes, I’m going to have to either shoehorn some raw SQL into the extension manager, or maybe even trust the extension developers to write the CREATE TABLE queries themselves!”.
Luckily, there’s a better alternative.
Django knows about its applications through a dictionary called INSTALLED_APPS. When you add a new model to a Django project, you simply add the model app to the INSTALLED_APPS dictionary, and run “manage.py syncdb”. Django does the magic, bingo-bango, and boom – tables created.
So if a new extension has some tables it needs created, I simply insert the app name of the extension into INSTALLED_APPS when the extension is installed, and call syncdb programmatically. Tables created: no sweat.
Creating tables is easy. But what if an extension gets updated, and the table needs to be modified? Sounds like we’ve got a mess on our hands.
And don’t expect Django to save you. When you modify a model in Django, they expect you to into that DB and alter that table by hand:
[syncdb] creates the tables if they don’t yet exist. Note that syncdb does not sync changes in models or deletions of models; if you make a change to a model or delete a model, and you want to update the database, syncdb will not handle that.
From The Django Book – Chapter 5: Models
Thankfully, there’s a mechanism that’s already built into Review Board that makes this trouble go away: django-evolution. Django-evolution, when used properly, will automatically detect changes in application models, and alter the database tables accordingly. This is how Review Board does upgrades.
And to top that off, RB co-founder Christian Hammond just became the django-evolution maintainer.
Wow. Everything is falling neatly into place.
So, after diving into the Review Board extension code, I hit a little snag.
It turns out I never learned about Python metaclasses. And the extension code in Djblets / Review Board uses them. In the map I made of the Review Board extension code, I represented my confusion with an image of some kind of quantum-divide-by-zero implosion.
I’ll get to the why for metaclasses in a second. First, I’ll demonstrate how it’s currently being used in the code.
This snippit is from the Djblets library, in the extensions folder, in base.py:
... class ExtensionHook(object): def __init__(self, extension): self.extension = extension self.extension.hooks.add(self) self.__class__.add_hook(self) def shutdown(self): self.__class__.remove_hook(self) class ExtensionHookPoint(type): def __init__(cls, name, bases, attrs): super(ExtensionHookPoint, cls).__init__(name, bases, attrs) if not hasattr(cls, "hooks"): cls.hooks = [] def add_hook(cls, hook): cls.hooks.append(hook) def remove_hook(cls, hook): cls.hooks.remove(hook) ...
And here are those classes being used in the Review Board extension directory, where it defines its hooks:
... class DashboardHook(ExtensionHook): __metaclass__ = ExtensionHookPoint def get_entries(self): raise NotImplemented class NavigationBarHook(ExtensionHook): """ A hook for adding entries to the main navigation bar. """ __metaclass__ = ExtensionHookPoint def get_entry(self): """ Returns the entry to add to the navigation bar. This should be a dict with the following keys: * `label`: The label to display * `url`: The URL to point to. """ raise NotImplemented ...
The idea here is that, if someone wanted to write an extension, they could add a hook to the navigation bar by subclassing the hooks, like so:
(from the rbreports prototype extension)
...
class ReportsDashboardHook(DashboardHook):
def get_entries(self):
return [{
'label': 'Reports',
'url': settings.SITE_ROOT + 'reports/',
}]
class ReportsExtension(Extension):
is_configurable = True
def __init__(self):
Extension.__init__(self)
self.url_hook = URLHook(self, patterns('',
(r'^reports/', include('rbreports.urls'))))
self.dashboard_hook = ReportsDashboardHook(self)
...
So you can see the __metaclass__ thing up there in the DashboardHook. DashboardHook subclasses ExtensionHook, and has ExtensionHookPoint as a metaclass.
Why? And what is a metaclass anyways? Why is this useful at all?
Well, luckily, I think I’ve figured it out.
Metaclasses are deeper magic than 99% of users should ever worry about. If you wonder whether you need them, you don’t (the people who actually need them know with certainty that they need them, and don’t need an explanation about why).
– Python Guru Tim Peters
There are plenty of resources available regarding Python’s metaclasses. And I ended up reading a ton of them trying to figure out just what the hell metaclasses actually do.
As it turns out, the best explanation I got was from Wikipedia:
In object-oriented programming, a metaclass is a class whose instances are classes. Just as an ordinary class defines the behavior of certain objects, a metaclass defines the behavior of certain classes and their instances.
In particular, check out their example on Cars. That, for me, more or less set the record straight on how metaclasses are used.
But why does Review Board use them when defining those hooks, like DashboardHook?
Forget the metaclasses for just a second.
Something is missing from that code that I posted up.
I’ll pose it as a question: How exactly is Review Board supposed to know about ReportsDashboardHook? Like…if we’re rendering the Dashboard, and want to fire off calls to all of our DashboardHooks…how do we do it? How can we find all of the active extension subclasses for DashboardHook?
One way you could do it, is to dive into the extensions directories, and use regular expressions to find defined classes. That sounds like a lot of work, brittle, and prone to error. What else?
Next, you could go back to those extension files, “eval” them (shudder), and extract the globals(), looking for ExtensionHook subclasses. That also sounds like lots of work, brittle, and prone to error.
You could go for class.__subclasses__…but this gives you every subclass, and we just want the hooks that are for active extensions. We could filter out all of the ones that aren’t activated, but we’d have to do that for every hook, and that blows.
Next, we could have developers add their hooks to a registry after defining them. So, we could have:
class ReportsDashboardHook(ExtensionHook): # ... # code goes here # ... dashboard_hook_list.append(ReportsDashboardHook)
And then when an extension is shut down, we just remove it from the global hook list. That doesn’t look so bad, does it? The only problem, is now we have to trust that extension developers will know to add those hooks to the global_hook_list? It’s another step, another thing to forget, and another point of failure.
And it seems silly – I mean, why go through all this effort? Review Board has already seen these hooks…why should it be so hard to just access the list activated hooks easily?
And there it is. That’s why I think we’re using metaclasses.
If you go back to ExtensionHook and ExtensionHookPoint, you’ll notice that ExtensionHookPoint has a list as an instance variable called hooks. That’s the global hook list for that class of hook (DashboardHook). And then ExtensionHook, in its constructor, automatically adds itself to the Extension’s internal list of hooks, as well as the global hook list, with the add_hook method. Shutting down the hook removes that hook from the global hook list.
So now, all we need to do to add our implemented hook to the list of hooks, is to simply subclass DashboardHook, or NavigationBarHook, or any of those Review Board hooks. No need to add the hook to the global list manually – Djblets / Review Board will take care of it. So now, we can get all of the DashboardHook subclasses for activated hooks, easily and consistently, with no extra work for the extension developer. Same with the NavigationBarHook subclasses.
Smooth as glass. Nice.
Anyhow, that’s my understanding of it. If I’m totally off, I’ll let you know when I find out.
According to the timeline, I’m still in the community-bonding period for GSoC. Coding for my project is supposed to start sometime towards the end of May.
So I’m using the time to do the following:
And this last point is the one I want to talk about.
An extension isn’t just some isolated piece of code that gets crammed into an application. When you’ve got multiple extensions already installed and running, installing and activating a new extension is like introducing a new animal into an ecosystem. You have to make sure that your new animal plays nice with the others, and that, in the morning, there won’t be a pile of rotting corpses where your application used to be.
I may have gotten carried away with my metaphor.
So let’s look at what I’m envisioning as the life-cycle for a Review Board extension. I’ll start right from the top.
Ideally, this will work as nicely as WordPress’s implementation: a Review Board administrator is given a catalog of extensions to choose from within the Administrator interface, and one click later, the desired extensions are downloaded and ready to be installed.
For all you system administrators out there, that last idea might make your toes curl. A Review Board administrator is not necessarily a system administrator, and the system administrator knows what he/she likes on their machine. An application that can go and download other applications can be dangerous. We have to ensure that the application that we’re downloading is the one we’re trying to download. The last thing we need is some man-in-the-middle to do something cute and bork the code review machine. And we want to ensure that the extension functions as advertised. No hidden features. No self-destruct mechanisms. No back doors.
Do I have a plan for this part? Well….no, not really. I don’t imagine I’ll get that far – I consider it a little out of my scope. So, for my project, I think it’ll satisfy if the system admin (or Review Board admin) can manually download the extension, decompress it, and place it where Review Board can work with it. That other stuff can come later (and I’ll try to design so that it can come later easily).
Ok, so at this point, we’ve got our extension downloaded in a place where Review Board can see it.
So now what?
Now we need to install the extension. To me, that means letting the extension put its roots into the RB install by creating database tables, preparing initial data, and generally doing everything to make conditions suitable for the extension to function.
When an extension install begins, I imagine it will consider the following questions (in no particular order):
Once those 3 questions are answered, and everything is looking good for the install, the extension will create the database tables it needs (if any) in order to function. If anything goes wrong during this process, the database changes will be rolled back, and the user will get a full read out about what went wrong.
If nothing goes wrong, the extension will be installed. The user might then be asked to set some initial operating parameters for the extension.
Ok great – the extension is installed. Now what?
For something that sounds so dramatic, the explanation about what happens is pretty short: Review Board simply becomes aware that the extension is activated, and passes data through the necessary hooks in order for the extension to function properly. Upon activation, the extension should do a quick double-check to ensure that all prerequisites for the extension have been met. This is because we can’t trust users to activate an extension immediately after downloading them.
While it’s activated, the extension will probably react to various events that happen on Review Board. Tables will be updated. View methods will be run. Templates will be rendered.
If anything, ever, goes horribly wrong with an extension, the following will happen:
The administrator could try to reactivate the extension at this point. They might try to contact the extension developer for support.
If the user wants to rid themselves of an extension, they must first deactivate it. This will put it (and its dependents) in the dormant state. It just switches them off, nothing else.
Deactivated extensions can then be un-installed. If the user chooses to un-install the extension, the extension database tables and settings will be wiped out.
The extension itself won’t be deleted though – at least, not within the scope of my project. The extension files will need to be removed from Review Board manually.
At this point, any dependents that this uninstalled extension had will no longer be able to be activated.
Anyhow, that’s how I envision the life-cycle. It’s my first go at it, so I’d love to hear some feedback if you have any.
 |
 |
 |
47 
3342 2733 727 3527 27
