Meeting Minutes March 17, 2013

Opening Announcements

We’re entering the final stretch. Make sure you get your status reports in on time, and make sure they’re posted to the reviewboard-students mailing list as well so we can see when they’re sent out. zz_nh2, ayrus12, Katkat and gregwym’s terms are up in few weeks. So it’s probably best to start shifting yourself to getting your patches out of WIP as soon as possible, to give us time to review them, to give you time to alter them, while not cramming everything into the last week.


Q: Where would I place the link for people to add checklist items? There are different kinds, should we use permissions?

A: Directly on the Checklist will be fine. Let’s start with individuals. There are companies with thousands of people using RB, and have their own breakdown of teams.


He would like to be reviewed, so he can make sure his first time hacking with backbone is on the right track.

Q: Should the new patch be over the previous patch that I had submitted?

A: If it’s a replacement for the old patch but working to the same milestone, use the existing one.

Q: There’s the new installation function within djblet’s /extensions/ This should be within a new request right?

A: Yes, that’s a good example of a standalone patch that can broken out.


She is going to focus on having the datagrid display without hijacking the review request list.

Q: Not completely sure what “continuation line under-indented for visual indent” means.

A: The “continuation line …”, that’s something pep8 likes to complain about, but in general, make sure it’s just looking like how the rest of the codebase does things.

Q: Should the dashboard reply list just display the non-comment reviews and maybe also indicate if the review has comments?

A: Filter reviews by doing base_reply_to__isnull=True. Replies, under the hood, are a “review” as well, but we don’t expose that.


Had a tough week. Got a bit time to work on Saturday.

Q: Adding markdown to comments breaks the banners. Why?

A: Should always use <pre> instand of <div>. <pre> allow html display. Moving away from pre is going to be a headache all around. Requires djblets changes, RB changes.. css..


Just a reminder that we’re coming up on the end of the term soon, and we typically get inundated with code reviews. If you want prompt feedback, get things up sooner rather than later. Let us know when you need a review and make sure things are out of WIP when they’re ready.

Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s