TFS code review should not require a new code review to be started when code that is already under review is changed based on review comment
It seems that TFS requires a new code review to be started whenever code that is already under review is updated in response to review comments. It is not only unnecessary busy work for a developer to initiate a new review each time a review comment would suggest a change to the code that is already under review is necessary, but it also prevents having a single picture of changes made as a result of the review process.
With the current method, a single logical code review may span multiple physical code reviews and the comments for the single logical review are fragmented across several physical reviews.
The current philosophy seems to be that only active team members can participate in code reviews which are always made for one check-in. Here are a few suggestions on how to make it more usable.
1) Make it web-based so one doesn't need Visual Studio (and sources for entire solution). This way one could easily ask for consultancy from a remote expert
2) Make it possible to create review request afterwards and possibly spanning multiple check-ins. The current practise suggests people to do huge check-ins which is not a best practise.
3) Have the comments to make a discussion tree. Currently comments are plain comments and one can't reply to a comment, so a real conversation is not possible using this feature.
4) The process of adding a comment (right click -> add comment -> write -> save (or using shortcuts left mouse click -> ctrl+shift+k -> write -> ctrl+enter) is not as fluent as it could be.
Just clicking the source in code review window could open a new comment box. And a plain enter on this box could save it.
We have a lot of investment taking place on our code review platform that will enable iterative code reviews, and we will initially bring that functionality to Git (via pull requests). Improving iterative code reviews for TFVC is still on our backlog, but it is not in our 6 month plan, so we are resetting the status.
Rob Siklos commented
@Andrew Ch: This is not a a reliable of doing things. The problem is that as the comments get addressed, the line numbers change, and you complete lose sight of what the original code review captured.
@80's Rocker: Couldn't agree more, but the writing is clearly on the wall - TFSVC is dead, and too bad for all of us that are using it. I guess we're just expected to migrate to Git or stay with TFSVC and live with a reduced feature set. I wish they'd just come right out and say this. At the end of the day, it seems the mob has spoken and Git is the winner.
80's Rocker commented
This is really sad that it on 1/15/16 this was still not on the roadmap to be implemented. Greate feature is being killed by poor implementation and not fixing a glaring feature shortcoming in a timeley mannner. The VS team obviosly uses GIT and that is why getting this implemented in GIT was a big priority instead of TFS. But by doing that they are ignoring a very big user base of TFS who don'want to switch over to GIT.
Jason Tremper commented
As an ex-microsoftie: PLEASE MAKE CODEFLOW PUBLICLY AVAILABLE :)
Andrew Ch commented
There seems to be a way of doing this already -- if you make your changes in response to the code review, and then save the changes back to the shelveset (set the name to the same name of the shelve set used for the code review).
This is a problem for our team.
I agree with Joshua, moving to git is just not an option for us at this point. We have many in house build process(es) built on top of tfs vc. This really needs to be planned and implemented and not just abandoned for git.
Brendan H. commented
Just a comment that this is working well for us now using TFS Git on premise! We do not use TFVC.
Joshua Drake commented
Abandoning your own products for Git seems to be a self-defeating strategy.
Vito DeCarlo commented
I agree. I'm not against git, but if that's where everything is heading, I'd just assume migrate now and get it over with.
Am I reading this right? That this is being done for Git, but not TFVC?
If so, I am a bit concerned. I did not believe the "TFVC is dying" statements. But if this is being done for Git and is not even road-mapped for TFVC, well that looks bad.
Makes it looks as if TFVC is a "second class" member of the team. At best.
Keith Culpepper commented
Any update on this? I agree that the code review should be updated instead of having to create a new review.
Post-commit code reviews (2) are really needed for us!
David Coulter commented
Put hover text on ALL grayed out items, so we will know how to ungray them and use them!!! you can start with "Request Review".
Mathias Broikman commented
2) Is needed to make code review usable.
"I really sometimes wonder wether VS team eats its own dog food. I doubt that the feature is usable in any non-trivial project."
Matt Luceen commented
My team is using the "update shelveset" method posted below and we hit a snag. Here's the scenario and two workarounds for the snag we hit:
1. Person 1 requests a code review from a shelveset.
2. Person 2 finds issues, adds comments, and selects "Finished (Needs Work)".
3. Person 1 makes the necessary changes and pushes them to the same shelveset used in step 1.
4. Person 1 adds a comment to the code review to notify the team that the code is ready to be reviewed again.
5. SNAG: Person 2 reviews the new code and it looks good. However, Person 2 has already finished the code review -- they are unable to select "Finished (Looks Good)".
WORKAROUND 1 - Don't ever finish a code review if the code needs work
Don't ever use "Finished (Needs Work)" or "Finished (With Comments)". If the code needs work, Person 2 should leave a "-1" top level comment on the code review and leave file level comment(s) describing the work needed.
WORKAROUND 2 - Use Excel to reset Person 2's response
1. Open the code review in Visual Studio.
2. Under Actions, click Open Work Item.
3. Click the LINKS tab on the bottom left of the work item.
4. Right-click Person 2's "Finished (Needs Work)" response and open it in Excel.
5. Click into the Assigned To cell and type Person 2's name.
6. Click into the State cell and use the dropdown to select "Requested".
7. Close Excel, clicking Yes to confirm that you want to publish your changes to the team project.
8. Person 2 is now able to select "Finished (Looks Good)".
There is a nice workaround with shelved changes:
Rob Siklos commented
This would definitely be something worth having.
Elvis Miranda Aramayo commented
Exactly my thoughts, we need this feature implemented ASAP, as a code reviewer find myself quite limited without this functionality.
Stefan Sieber commented
We have given up waiting for code review to become usable. 2) was a complete killer for us. We're now using review assistant and it meets our requirements.
I really sometimes wonder wether VS team eats its own dog food. I doubt that the feature is usable in any non-trivial project.
Ryan Turner commented
Could not agree more with the original suggestions. These are all major pain points for developers. Managing multiple reviews of the same set of 10+ files becomes unfeasible with the current tool. Not being able to maintain a history within a single review and see diffs between different versions of files makes the code review process time-consuming, and traceability becomes a nightmare.