Support multiple review files

[girivs82]

🧩 Feature

Description

Currently, a single csv is generated in a user's workspace to hold comments. In order to allow multi-user reviews, there are a few things that perhaps should be done.

1. Add the csv review file to version control(assuming code is version controlled) and keep it sync'd across different users so that each is able to see the others comments. This could introduce version control related issues like merge conflicts etc., but since there is already a UUID based "id" field in the csv file, no two comments will have the same id, eliminating a merge conflict issue.
2. Outside changes to .csv files must be detected and the UI should reflect the latest changes to the csv file, since in the multi-user case, the extension is not the only one changing the file, it can be changed by another user and updated when the workspace is sync'd.
3. When a review is closed and a new review is begun, it may be necessary to preserve the contents of the previous review for audit and/or traceability purposes. So I propose a new csv file for every new review. The filename can be based on an issue ID from an issue tracker like JIRA or it could be as simple as a UUID4 to ensure each review gets a new csv file name.

Feel free to let me know if this sounds too complicated. The other option is for the extension to integrate with various other production ready code review systems like SmartBear's code collaborator, Atlassian crucible, perforce swarms etc.

Thoughts?

[d-koppenhagen]

Hey @girivs82 thanks for your feature suggestion.

I am not sure whats currently missing for your feature or I didn't understand the real use-case. Can you describe it a bit more? some thoughts related to your points:

1. Add the csv review file to version control(assuming code is version controlled) and keep it sync'd across different users so that each is able to see the others comments. This could introduce version control related issues like merge conflicts etc., but since there is already a UUID based "id" field in the csv file, no two comments will have the same id, eliminating a merge conflict issue.

Putting a review file under version control is something project related and must be done by a user musn't it? So the extension just analyzes the review file and puts comments into context. You are right: when multiple users working on the project concurrently each new review comment will create a new CSV line containing a unique ID. So there should be any conflict I think.

2. Outside changes to .csv files must be detected and the UI should reflect the latest changes to the csv file, since in the multi-user case, the extension is not the only one changing the file, it can be changed by another user and updated when the workspace is sync'd.

I just tried it out as I was pretty sure I already implemented file watchers. So in fact: When you edit the review file manually and you will add valid lines / remove some, changes will be registered and reflected in the comment explorer as well as in the file annotations itself. So nothing to do here right?

3. When a review is closed and a new review is begun, it may be necessary to preserve the contents of the previous review for audit and/or traceability purposes. So I propose a new csv file for every new review. The filename can be based on an issue ID from an issue tracker like JIRA or it could be as simple as a UUID4 to ensure each review gets a new csv file name.

not sure what you mean with "a review is closed"? Do you mean the review file is removed as all issues are resolved? Or just a review file with different name is created? So indeed you can start your review and once you finished it, you can just move the file elsewhere, then the comments are preserved, aren't they? Or you can change the settings.json and change the default review file-name. One thing I can imagine, is to detect multiple review files on the disk and then display all contents in the comment-view explorer. But supporting multiple review files at the same time would affect almost every feature I think as you should always be able to choose the right one before create a comment export to some format.

cheers, Danny

[girivs82]

Thanks for the quick response Danny!

My responses in-line.

Hey @girivs82 thanks for your feature suggestion.

I am not sure whats currently missing for your feature or I didn't understand the real use-case. Can you describe it a bit more? some thoughts related to your points:

Add the csv review file to version control(assuming code is version controlled) and keep it sync'd across different users so that each is able to see the others comments. This could introduce version control related issues like merge conflicts etc., but since there is already a UUID based "id" field in the csv file, no two comments will have the same id, eliminating a merge conflict issue. Putting a review file under version control is something project related and must be done by a user musn't it? So the extension just analyzes the review file and puts comments into context. You are right: when multiple users working on the project concurrently each new review comment will create a new CSV line containing a unique ID. So there should be any conflict I think. [girivs82] I agree, but I was thinking more along the lines of the extension actually doing an active role in merging the file back and forth between different users, but sure, it can be done manually; let us assume we can do this manually and leave it at that for now.

Outside changes to .csv files must be detected and the UI should reflect the latest changes to the csv file, since in the multi-user case, the extension is not the only one changing the file, it can be changed by another user and updated when the workspace is sync'd. I just tried it out as I was pretty sure I already implemented file watchers. So in fact: When you edit the review file manually and you will add valid lines / remove some, changes will be registered and reflected in the comment explorer as well as in the file annotations itself. So nothing to do here right? [girivs82] You are right. If file watchers are already implemented and the comment explorer and annotations get updated as soon as the csv is updated in disk, there is nothing more to do.

When a review is closed and a new review is begun, it may be necessary to preserve the contents of the previous review for audit and/or traceability purposes. So I propose a new csv file for every new review. The filename can be based on an issue ID from an issue tracker like JIRA or it could be as simple as a UUID4 to ensure each review gets a new csv file name. not sure what you mean with "a review is closed"? Do you mean the review file is removed as all issues are resolved? Or just a review file with different name is created? [girivs82] When all issues are resolved and approved is what I mentioned as review "closed"

So indeed you can start your review and once you finished it, you can just move the file elsewhere, then the comments are preserved, aren't they? Or you can change the settings.json and change the default review file-name. One thing I can imagine, is to detect multiple review files on the disk and then display all contents in the comment-view explorer. But supporting multiple review files at the same time would affect almost every feature I think as you should always be able to choose the right one before create a comment export to some format. [girivs82] Indeed, this was one thing I was thinking of since I could be a reviewer for one review while being the author of another review concurrently. So being able to context switch here would be very useful.

cheers, Danny

[d-koppenhagen]

@girivs82 I agree, it would be useful to implement a feature that allows to switch review files seamlessly. So let's focus on this. Meanwhile I fixed a small bug where the decorations weren't updated correctly on review comment changes / deletion.

[d-koppenhagen]

So what do you think about the following:

Currently the filename for a single review file is defined via code-review.filename wich accepts a string. In order to support multiple review files and being backwards-compatible, I would say, for normal purpose, the seeting can stay as it is. But: Users can change this setting to an array of review filename strings. Once the extension detects, the setting contains an array of strings, the mutli-mode is active which means: When the array contains more than one review file reference and an an actions tries to get the filename, this will be intercepted by vscode promt which let's users choose between all the referenced files. Also a new command could be implemented that creates a new review file with a given name and enentualle converts the existing string parameter for the filename to an array containing the previous string and the newly created review filename.

One thing I am still thinking of is the comment explorer. There could be an extra heirarchy level that will group all comments based on the review filename. Another solution would be that a user should choose the file to display immediatly after clicking on the comment explorer. What do you think?

[girivs82]

I like this approach.

1. Use code-review.csv is the default file for backwards compatibility and to keep existing behavior
2. Once changed to array, allow prompts to choose which file to use. One suggestion here is to implement named reviews; so that it is easily seen in hierarchy what each review file is for eg:- Library FOO; Tool XYZ; Refactor ABC...; or some people might prefer a JIRA ID as the review name for example.
3. Regarding the comment explorer, I orefer the hierarchical view, but would also be ok to choose a file and have it display only comments related to that file.

Thanks! Shankar

Shankar Giri V - Chat @ Spike [15r91k]

On August 13, 2021 at 8:39 GMT, Danny Koppenhagen @.***> wrote:

So what do you think about the following:

Currently the filename for a single review file is defined via code-review.filename wich accepts a string. In order to support multiple review files and being backwards-compatible, I would say, for normal purpose, the seeting can stay as it is. But: Users can change this setting to an array of review filename strings. Once the extension detects, the setting contains an array of strings, the mutli-mode is active which means: When the array contains more than one review file reference and an an actions tries to get the filename, this will be intercepted by vscode promt which let's users choose between all the referenced files. Also a new command could be implemented that creates a new review file with a given name and enentualle converts the existing string parameter for the filename to an array containing the previous string and the newly created review filename.

One thing I am still thinking of is the comment explorer. There could be an extra heirarchy level that will group all comments based on the review filename. Another solution would be that a user should choose the file to display immediatly after clicking on the comment explorer. What do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.