google / git-appraise

Distributed code review system for Git repos
Apache License 2.0
5.12k stars 145 forks source link

Handle Comment on Simultaneous Reviews Request #117

Open rfon6ngy opened 5 months ago

rfon6ngy commented 5 months ago

I'm currently Implementing a git-appraise extension for VSCode and I'd like to clarify the following:

ojarjur commented 5 months ago

Thanks for reaching out @134744072 , I'll respond as best as a can.

I'm currently Implementing a git-appraise extension for VSCode

Cool! That sounds like a great project and would love to see what you get when you are ready to share it.

It's impossible to have simultaneous opened review on a commit => can we use comment.parent to point to the parent request sha1 ? similar to what is currently done on the comment-comment relation.

This is a good point; I don't think we do have a good way to support that right now.

The idea of a parent-child relationship for sub-reviews is interesting, but I'm not sure exactly what the end user experience for that would look like.

Can you describe from the user's perspective how they would use this? Perhaps something like a mock user documentation explaining how they would use the feature.

git-appraise list -a report an empty list on git 1.8.3.1 (RHEL7)

This will probably need to be tracked in a separate, stand alone issue. In particular, I'd like to see an end-to-end reproduction steps.

Can you try to create new, temporary directory, initialize a git repo in it, create some reviews, and then reproduce the issue? If so, please list all of the steps you took and what you see vs. what you expect (again, this should probably be a separate issue).

Why message schema don't specify that lines/columns location are 1-based

They aren't; they are 0-based. They specify the number of lines/columns that come before the location.

For example, to add a comment before the first line, the line number would be 0, but to add a comment after the first line and before the second, the line number would be 1.

Are additionalProperties allowed on the request and message JSON ?

That should work gracefully (e.g. the golang code in this repository should just ignore the additional properties).

If you find it doesn't work like that, then please file a bug.

rfon6ngy commented 5 months ago

Hi @ojarjur ! Thanks for the reply

I'll investigate the git v1.8 issues and report that in a separate issue.

For the multiple request on 1 commit issues, do you recommend

the latest seems to be the most tool-compatible.

Would you mind if I PR you an updated schemas with added additionalProperties and "0-based location" precisions ?