Bugzilla:Review

From MozillaWiki
Jump to: navigation, search

In Bugzilla, almost every patch that is submitted goes through a process of review, where it is looked at by an experienced Bugzilla developer to make sure that it meets our standards for code quality.

The review process is very important. Without it, Bugzilla's code would quickly become a terrible mess.

How to Ask for Review

When you attach your patch, you'll see a "review" flag. You want to set this to "?" and put the username of the correct Bugzilla reviewer in the box next to the "?".

How do you know who's the right reviewer? Use the List of Reviewers, which explains the whole thing.

See also: Code Review FAQ.

How the Review Process Works

If the reviewer thinks your patch is acceptable, he will change the review flag to a "+". If there are things you need to fix, the reviewer will mark it with a "-".

If there are things you need to fix before we can accept the patch, the reviewer will change the review flag to a "-". He/she will include comments about what needs to be fixed. You should attach a new patch with those things fixed, and request review again.

Sometimes the reviewer will prefix his comments with "Nit:". This means that he's just "nitpicking"--you don't have to fix these points, but we'd like you to.

If you disagree with a review, just respond to it in the comments! This isn't a dictatorship, it's an open-source community.

Who Fixes The Patch?

The author of the patch is usually the one who revises the patch if it gets a "-" on its review. The reviewer probably could fix your patch, but every Bugzilla reviewer is very busy and doesn't have the time or the desire to do so.

Also, it's a better learning experience for you to fix the patch, not to mention we tend to assume that most people want to take sole credit for their work, and many of us feel that we'd be stepping on your toes if we took over working on it.

Basically, we look at your patches as your responsibility. Once they are attached, it's up to you to revise them until we accept them.

If you really want to just contribute what you've done so far and let someone else clean it up and push it through the review process, say so on the bug. There's no guarantee someone else will pick it up and run with it, and you're greatly decreasing the chances of the patch making it into our code repository by doing so, but announcing that you'd like someone else to take it over is much better than a patch sitting there for 6 months because everyone is assuming the patch author will be back to fix it.

What If I'm Waiting Too Long for My Review?

If you've waited longer than a week for your review, come into IRC and bug us. If you can't get a hold of your reviewer on IRC, just send him or her an email.

If you wait longer than two weeks for your review, pick another reviewer from the reviewer list. If there isn't another reviewer, just keep communicating with the current one in IRC or by email until you get a result.

It's Not a Personal Criticism

All of us work very hard on our code, and we all take pride in our work. Sometimes, getting criticized for what we've done is difficult, particularly when we've spent a lot of time on it.

Bugzilla reviewers may seem to be harsh, accidentally. They aren't trying to be harsh or overly critical, they're just pointing out what needs to be changed, which usually means they're pointing out what's wrong with the current code, instead of pointing out what's right with it. Usually they don't have lots of extra time in their life for reviews, so they just quickly write what needs to be fixed, without spending too much time thinking about the nicest way to say it. Sometimes they also don't go into long explanations.

You're not a terrible programmer or a bad person. All we're doing is telling you what's preventing us from checking the code into Bugzilla's main codebase. It's not even that you're wrong, it's just that there are certain things we need before checking in code into Git. Sometimes those requirements may seem unusually strict to new contributors--trust me, they're necessary, from long experience working on Bugzilla. They may seem hard now, but you'll understand after a while of working on the Project.

And remember, anything we didn't say was wrong, that was good code!

Keep on submitting patches, and you'll get better in time, and reviews will get easier and easier. Even if sometimes it might seem hard, it's a great learning experience, and it really makes Bugzilla a better product.