Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Comment on Simultaneous Reviews Request #117

Open
rfon6ngy opened this issue Feb 10, 2024 · 2 comments
Open

Handle Comment on Simultaneous Reviews Request #117

rfon6ngy opened this issue Feb 10, 2024 · 2 comments

Comments

@rfon6ngy
Copy link

rfon6ngy commented Feb 10, 2024

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

  • 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.

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

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

  • Are additionalProperties allowed on the request and message JSON ?

@ojarjur
Copy link
Collaborator

ojarjur commented Feb 11, 2024

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
Copy link
Author

rfon6ngy commented Feb 12, 2024

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

  • to reuse the parentId
  • create an extra parentRequest ?

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants