Confirmed users
955
edits
Bugzilla:Review: Difference between revisions
m
→It's Not a Personal Criticism: bazaar -> git
No edit summary |
m (→It's Not a Personal Criticism: bazaar -> git) |
||
| (6 intermediate revisions by 3 users not shown) | |||
| Line 1: | Line 1: | ||
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. | 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. | The review process is ''very important''. Without it, Bugzilla's code would quickly become a terrible mess. | ||
== How to Ask for Review == | == How to Ask for Review == | ||
| Line 8: | Line 8: | ||
How do you know who's the right reviewer? Use the [http://www.bugzilla.org/docs/reviewer-list.html List of Reviewers], which explains the whole thing. | How do you know who's the right reviewer? Use the [http://www.bugzilla.org/docs/reviewer-list.html List of Reviewers], which explains the whole thing. | ||
See also: [https://developer.mozilla.org/en-US/docs/Code_Review_FAQ Code Review FAQ]. | |||
== How the Review Process Works == | == How the Review Process Works == | ||
| Line 13: | Line 15: | ||
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 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. | 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. | 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. | ||
| Line 21: | Line 23: | ||
=== Who Fixes The Patch? === | === Who Fixes The Patch? === | ||
'''The author of the patch''' is | '''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. | 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. | 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? === | === What If I'm Waiting Too Long for My Review? === | ||
| Line 39: | Line 43: | ||
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. | 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 code into | 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! | 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. | 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. | ||
[[Category:Bugzilla|R]] | |||