Skip to content

Partly fix incorrect code review comment line number #31301

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

Closed
wants to merge 11 commits into from

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Jun 10, 2024

A quick fix for #30844

The solution is updating the comment line when load diff.
We will get the diffs from the commit SHA of this comment when it is created to the current PR's commit SHA.
Then we can get the correct line number. (whether the commented line is deleted or becomes to another line)

But this will increase the I/O cost.

TODO

  • when add comments still have a bug: hidden comments will display
  • fix test

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 10, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jun 10, 2024
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 10, 2024
@github-actions github-actions bot added modifies/templates This PR modifies the template files modifies/js labels Jun 10, 2024
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 10, 2024
@yp05327 yp05327 closed this Jun 18, 2024
@skarndev
Copy link

skarndev commented Jun 18, 2024

Why closed?

@yp05327
Copy link
Contributor Author

yp05327 commented Jun 20, 2024

  1. The solution of this PR is not good enough. We need to update the line when user pushes new commits.
  2. I have tried to rewrite a new one. But I notice that the current DB design is not good enough. (It is related to 3) We need to discuss it.
  3. There's no template ( a correct rule to display these line numbers ), as GitHub also have some similar bugs, so we should decide them first. e.g.
  • When should we remove the comment ? Do we need to recover the comment after user deleted and re-added the changes again. e.g. line 6 + aaa there's a comment, then the next commit removed this line, so this comment will also be removed, and then the next next commit added this change back ( same line number or different line number ), do we need to show this comment again or not. And also another question : should this comment keeping outdated status?
  • How to pin the line number to the comment, IMO, we need to save the line number when the comment created ( this is what we did ), and also the line number of the latest status.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants