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

Code review tool git-appraise intergration #189

Closed
lunny opened this issue Nov 17, 2016 · 17 comments
Closed

Code review tool git-appraise intergration #189

lunny opened this issue Nov 17, 2016 · 17 comments
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@lunny
Copy link
Member

lunny commented Nov 17, 2016

Since git-appraise will store code review data in refs/notes/devtools, gogs can read the data and show info on the web interface.

Migrated from gogs/gogs#2210

@lunny lunny added type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Nov 17, 2016
@lunny lunny added this to the 1.x.x milestone Nov 17, 2016
@bkcsoft
Copy link
Member

bkcsoft commented Nov 29, 2016

I'll read up on git-appraise tomorrow, the README looks promising though :)

@stevenroose
Copy link

@bkcsoft You read and decided that it was too much work? :)

@bkcsoft
Copy link
Member

bkcsoft commented Dec 26, 2016

It just feels like it will break very fast. How does it handle "merge-conflicts" etc?

@ojarjur
Copy link

ojarjur commented Jan 3, 2017

@bkcsoft The metadata formats used by git-appraise are designed to work with the git-notes 'cat_sort_uniq' merge strategy, so merge conflicts are not an issue (they are automatically resolved by the git notes merge command).

@ojarjur
Copy link

ojarjur commented Jan 4, 2017

@bkcsoft I realized a little earlier that what you were worried about with merge conflicts might have been "what happens when a comment is edited?", and that deserves a bit more details:

The formats git-appraise uses do not represent the current state of a review, but rather a log of events (e.g. "User A added a comment at line B").

This means we can automatically resolve conflicts because we can take the union of all events (this is what the 'cat_sort_uniq' strategy gives us).

However, there is a gotcha in that the current set of events does not include an 'edit comment' event (it does support 'edit review request' events).

I've filed this issue to add such support.

@stevenroose
Copy link

@ojarjur Wow, it's a real honor to have you here as the original git-appraise author! Hopefully collaboration will allow this issue to move forward more quickly!

@choucavalier
Copy link

What is the current status of this?

@lunny
Copy link
Member Author

lunny commented Jan 22, 2017

Waiting for someone send PR. :)

sokolovstas added a commit to sokolovstas/gitea that referenced this issue Jan 23, 2017
@lunny lunny modified the milestones: 1.1.0, 1.x.x Jan 26, 2017
@lunny lunny modified the milestones: 1.2.0, 1.1.0 Feb 11, 2017
@lunny lunny modified the milestones: 1.x.x, 1.2.0 Apr 20, 2017
lafriks pushed a commit to lafriks-fork/gitea that referenced this issue May 21, 2017
ethantkoenig pushed a commit to ethantkoenig/gitea that referenced this issue Jun 4, 2017
@vizcay
Copy link

vizcay commented Sep 6, 2017

There is someone currently working on this? There is https://github.com/google/git-appraise-web to use as an starting point..

@strk
Copy link
Member

strk commented Sep 6, 2017

See #733 for an ongoing effort (stuck, as far as I can tell)

@vizcay
Copy link

vizcay commented Sep 12, 2017

It will be nice until #733 gets developed to be able to reference sections of code in the discussion of the pull request. This should be very easy to implement:

  • Add a small "copy link" in the ruler to the left of each file to reference that exact position, to be able to paste it in the comments.
  • Via js when the user clicks that link it navigates to that section of the code (even highlight it maybe).

That way it will be possible to discuss sections of code more easily, that's what people is interested in general I believe.

@OmarAssadi
Copy link
Contributor

Would definitely be nice to see some sort of code review in Gitea. Added onto the original bounty. By the way, your bountysource badge appears to be broken!

Here is the current total: current amount

@ypnos
Copy link

ypnos commented Jan 5, 2018

The bounty is now over $200. Would be great if somebody finds the time to get this killer feature (code review) done. A good start for Gitea into 2018!

@hickscorp
Copy link

hickscorp commented Mar 15, 2018

Hey - picking up the discussion on this very important feature, it's good to see that there is some activity.
I see that the bounty is right now $210, what do you guys think it should reach to potentially be fair to pay whoever will be contributing to this (Trying to get a feel of the general consensus here not of a price tag)?

@lunny BTW thanks for taking a stab at it, even if it didn't pan out like something usable. At least you've shown everyone else that the comments should be per-pull-request, not global.

Also, @ojarjur being around this discussion feed, I'm pretty sure he'd give more advice about git-appraise if needed.

Also, and given the status of #733, I think a specification should be established before the work order is issued - as that last PR got stuck because inline comments were not per-pull-request.

As @vizcay suggests, there are two things that could be useful here:

    1. The actual pull-request review flow, with inline comments, that would involve backend and database work,
    1. The ability to easily manipulate line references in comments in general (including other parts of the app), using a more client-side href / anchor thing.

I also agree with @vizcay in the sense that the 2nd point is a very good way to mitigate the lack of the 1st point, as what the users are aiming for is a way to collaborate and talk about code-points in particular - and I can definitively see myself following a discussion on the main PR page while Ctrl+Clicking line numbers.

Therefore, another and 3rd way of mitigating both things would be an in-between solution. How about:

  • Whenever someone references a line of code in a comment, then the backend keeps track of those in a dedicated table, and potentially computes the composite data required for fast indexing and later query "per-file",
  • Then when the backend generates the output for a file, it would place a bullet on the left margin if that file + line was referenced in a comment, allowing to click it and jump to that (those) particular comment(s).

This way, the PR main screen would still syndicate discussion, while file views would stick to do what they do - viewing files.

I guess that later approach could be also an innovative way of doing what users are asking for, without all the burden of the complete task, while maybe differentiating Gitea from the others ;)

EDIT: BTW I absolutely know how frustrating it is as a maintainer to have peeps +1 a feature request all the time - while they could be doing it themselves. That is not the intent of this comment, I'm indeed available for helping more in the thinking process. I gave up on Golang years ago tho, so I won't produce any code at all as I'm way too rusty (Or should I say elixiry). I could also tip-in, if that's ultimately what's missing.

@Russtopia
Copy link

Russtopia commented Dec 20, 2019

Hi all,

This is sort of a crazy idea, but ... perhaps a solution for code review that is decoupled from any particular platform could be made from one of the web plugins that allow general annotation on top of HTML, such as https://web.hypothes.is/ or some of the others here: https://www.maketecheasier.com/google-chrome-extensions-annotate-text-on-the-web/

Perhaps someone with knowledge in the domain of web extensions could customize such an already-existing tool to structure the highlighting/annotation features specifically for code review, but I've already tried hypothes.is out on a pull request on my own gogs instance, and it supports public, group and private annotations with nice navigation, so it's 90% there already. Wonder if it could be self-hosted or co-hosted with one's gogs/gitea instance for annotation storage...

@techknowlogick
Copy link
Member

Closing as code reviews exist.

@lafriks lafriks removed this from the 1.x.x milestone Dec 9, 2020
@ypnos
Copy link

ypnos commented Jan 1, 2021

The issue is rightfully closed, but the bounty is still unclaimed: https://www.bountysource.com/issues/39308681-code-review-tool-git-appraise-intergration

Me personally I would be fine if whoever implemented code review, claims this bounty. @lafriks that would be you, right?
Certainly I would not like the money to go to waste (eaten up by Bountysource, as they have already threatened with their, for now retracted, new ToS).

For reference: #3748

@go-gitea go-gitea locked and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

No branches or pull requests