Code review for files, not only for changesets

Issue #511 resolved
Adam Dawidziuk created an issue

I've been playing around with RhodeCode for the last few days and truth be told there is one important thing missing when it comes to code-review: mainly RH lacks the possibility of reviewing the entire files (so their state at a TIP). I trust such a functionality would benefit RH greatly and create an even more positive distinction from "all other CR systems out there". Plus a better information/indication layout (most notably in group/repository view) for: - tasks undergoing code review (just an icon indicating this changeset/file is being reviewed) - tasks with comments (similarly as before)

Did you though about that functionality, or perhaps it's already on the roadmap? ;>

Comments (5)

  1. Marcin Kuzminski repo owner

    Yes, i was thinking about that, but there are several issues with reviewing files at 'tip' Most important, after you review lines for file, next commit can remove/add more lines, then inline comments become invalid, and they will be lost.

    Do you have an idea how file review might work ? I'm open for discussions.

  2. domruf

    +1 I think the best way would be to (optionally) show the whole file during chgeset reviews. And also allow to review while browsing the current files. But the notifications should lead to the review of the specific changeset not the tip. Meaning if there are new changesets, the comments should no longer being shown on the tip. (at least not if the commented lines are altered or deleted)

  3. Adam Dawidziuk reporter

    Throwing some ideas for review (I'm aware they might cause few side-effects here and there)

    Review the file at tip in "annotate view" - so basically you have the possibility to click/comment on whatever line of code, but underneath the comment will actually be added to a changeset associated with this particular line. Obviously the problem with that is how to merge all comments from all changesets to a unified view. Much like replicating VCS job...

    Anyhow then if you modify/remove a line with a new patch it will basically cover/overlap the previous state at that line thus perhaps hiding the old comment in "annotate view" and allowing to add another one. Other than that comments for "overloaded" lines should still be available when accessed directly from old changesets.

    So in short we can consider this method to be a merge of all comments for all changesets. I assume it's not trivial though.

    Unfortunately I don't think there is an ultimate solution that will satisfy everyone and every use case - much like with any VCS - you can try to do cherry-picking without any proof it makes sense syntactically/semantically (it probably doesn't)

  4. Log in to comment