4 ms·

What were the bad habits in code review that you were thinking about? I'm stuck with TFVC and the code review in TFS is very poor but I used ReviewBoard about 8

by bogle 8y ago

What were the bad habits in code review that you were thinking about? I'm stuck with TFVC and the code review in TFS is very poor but I used ReviewBoard about 8 years ago and thought it was the best F/OSS available at the time.

chipx86 8y ago
Not to hijack the thread too much (I'm also very curious about the answer -- I'm one of the Review Board founders and these discussions always interest me), but we support TFS these days in Review Board by way of our Power Pack add-on product, if you'd like an alternative solution. I can talk to you more about this off this thread if it's at all interesting to you.
guitarbill 8y ago
mainly for CRUX and the surrounding ecosystem: it's very hard to protect the main branch from pushes that don't meet approval, that's done afterwards during deployments. this is obviously and horrendously dumb, you really want to catch stuff before it goes in master/mainline, and having to fix this stuff before deploying it also a great use of time /s. but it gets worse. the UI can't or won't do trivial rebases. so either you upload a new, rebased revision, and get the reviewers to re-review (because stale reviews get wiped), or just rebase, push, and mark as merged. i get you are supposed to review rebases, but this can grind a team to a halt. you're waiting for approvals, but by the time you get them someone else has pushed a new change... a colossal waste of time (edit: especially if you require 2 approvals, which i do think is good practice, and some AWS teams do it). this also means people are trained to ignore the merge button or expect it to be greyed out. but it might be greyed out for other reason though. in GitHub, you have big green check-marks or red icons to indicate which conditions have and haven't been met (reviewers, CI, merge conflicts). in CRUX, hovering over the greyed out merge button gets you one cause at best (there is a page/tab that shows more detail, but almost nobody looks at it). obviously it depends on your team, but it requires near constant vigilance to maintain high quality with CRUX, especially with a big team or less experienced devs contributing. process is supposed to make quality easier, not harder! there are many other smaller issues, but that's the big one IMO. as for ReviewBoard, personally i prefer Gerrit, which itself doesn't have a great UI, but makes up for it in the workflow.
- hopler 8y ago
  Those complaints sound like CRUX issues not ReviewBoard. Looking at multiple branches is not in scope of ReviewBoard at all.
- someoneelse67 8y ago
  But you actually can setup rules to prevent merges unless certain people/groups have approved. It sounds like you and your team are suffering more from bad code review habits than a bad tool. Devs should review what they are publishing to their peers at least once before actually hitting the publish button. It’s a courtesy to them and their time I personally love the Crux tool and some of its add ons.
  - guitarbill 8y ago
    > But you actually can setup rules to prevent merges unless certain people/groups have approved. As detailed, that only prevents merging via the UX, not pushes to the main branch. Maybe the team does suck, but I still consider it a shortcoming of CRUX et al.

by bogle 8y ago

chipx86 8y ago

Not to hijack the thread too much (I'm also very curious about the answer -- I'm one of the Review Board founders and these discussions always interest me), but we support TFS these days in Review Board by way of our Power Pack add-on product, if you'd like an alternative solution. I can talk to you more about this off this thread if it's at all interesting to you.

guitarbill 8y ago

mainly for CRUX and the surrounding ecosystem: it's very hard to protect the main branch from pushes that don't meet approval, that's done afterwards during deployments. this is obviously and horrendously dumb, you really want to catch stuff before it goes in master/mainline, and having to fix this stuff before deploying it also a great use of time /s. but it gets worse. the UI can't or won't do trivial rebases. so either you upload a new, rebased revision, and get the reviewers to re-review (because stale reviews get wiped), or just rebase, push, and mark as merged. i get you are supposed to review rebases, but this can grind a team to a halt. you're waiting for approvals, but by the time you get them someone else has pushed a new change... a colossal waste of time (edit: especially if you require 2 approvals, which i do think is good practice, and some AWS teams do it). this also means people are trained to ignore the merge button or expect it to be greyed out. but it might be greyed out for other reason though. in GitHub, you have big green check-marks or red icons to indicate which conditions have and haven't been met (reviewers, CI, merge conflicts). in CRUX, hovering over the greyed out merge button gets you one cause at best (there is a page/tab that shows more detail, but almost nobody looks at it). obviously it depends on your team, but it requires near constant vigilance to maintain high quality with CRUX, especially with a big team or less experienced devs contributing. process is supposed to make quality easier, not harder! there are many other smaller issues, but that's the big one IMO. as for ReviewBoard, personally i prefer Gerrit, which itself doesn't have a great UI, but makes up for it in the workflow.

hopler 8y ago
Those complaints sound like CRUX issues not ReviewBoard. Looking at multiple branches is not in scope of ReviewBoard at all.
someoneelse67 8y ago
But you actually can setup rules to prevent merges unless certain people/groups have approved. It sounds like you and your team are suffering more from bad code review habits than a bad tool. Devs should review what they are publishing to their peers at least once before actually hitting the publish button. It’s a courtesy to them and their time I personally love the Crux tool and some of its add ons.
- guitarbill 8y ago
  > But you actually can setup rules to prevent merges unless certain people/groups have approved. As detailed, that only prevents merging via the UX, not pushes to the main branch. Maybe the team does suck, but I still consider it a shortcoming of CRUX et al.