search

google / git-appraise

Distributed code review system for Git repos
Apache License 2.0
5.14k stars 146 forks source link

The show command does not display comment status correctly #22

Closed jishi9 closed 8 years ago

jishi9 commented 8 years ago

It seems that the show command always displays the status of a comment as fyi.

Steps to reproduce

git appraise comment -m 'fyi comment'
git appraise comment -lgtm -m 'lgtm comment'
git appraise comment -nmw -m 'nmw comment'

Then running git appraise show will show something like:

    comment: $SHA
      author: $AUTHOR
      time:   $TIMESTAMP
      status: fyi
      fyi comment

    comment: $SHA
      author: $AUTHOR
      time:   $TIMESTAMP
      status: fyi
      lgtm comment

    comment: $SHA
      author: $AUTHOR
      time:   $TIMESTAMP
      status: fyi
      nmw comment

On the other hand git appraise show -json displays the correct status (in the resolved property):

  "comments": [
    {
      "hash": "$SHA",
      "comment": {
        "timestamp": "$TIMESTAMP",
        "author": "$AUTHOR",
        "location": {
          "commit": "$SHA"
        },
        "description": "fyi comment"
      }
    },
    {
      "hash": "$SHA",
      "comment": {
        "timestamp": "$TIMESTAMP",
        "author": "$AUTHOR",
        "location": {
          "commit": "$SHA"
        },
        "description": "lgtm comment"
        "resolved": true
      }
    },
    {
      "hash": "$SHA",
      "comment": {
        "timestamp": "$TIMESTAMP",
        "author": "$AUTHOR",
        "location": {
          "commit": "$SHA"
        },
        "description": "nmw comment"
        "resolved": false
      }
    }
barbastan commented 8 years ago

Interesting! Omar (our Tech Lead) wrote that code. Maaaaybe he had a reason. He'll be back on Jan 4 and will respond.

Thanks! Barbara

On Wed, Dec 23, 2015 at 4:25 AM, jishi9 notifications@github.com wrote:

It seems that the show command always displays the status of a comment as fyi. Steps to reproduce

git appraise comment -m 'fyi comment' git appraise comment -lgtm -m 'lgtm comment' git appraise comment -nmw -m 'nmw comment'

Then running git appraise show will show something like:

comment: $SHA
  author: $AUTHOR
  time:   $TIMESTAMP
  status: fyi
  fyi comment

comment: $SHA
  author: $AUTHOR
  time:   $TIMESTAMP
  status: fyi
  lgtm comment

comment: $SHA
  author: $AUTHOR
  time:   $TIMESTAMP
  status: fyi
  nmw comment

On the other hand git appraise show -json displays the correct status (in the resolved property):

"comments": [ { "hash": "$SHA", "comment": { "timestamp": "$TIMESTAMP", "author": "$AUTHOR", "location": { "commit": "$SHA" }, "description": "fyi comment" } }, { "hash": "$SHA", "comment": { "timestamp": "$TIMESTAMP", "author": "$AUTHOR", "location": { "commit": "$SHA" }, "description": "lgtm comment" "resolved": true } }, { "hash": "$SHA", "comment": { "timestamp": "$TIMESTAMP", "author": "$AUTHOR", "location": { "commit": "$SHA" }, "description": "nmw comment" "resolved": false } }

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/issues/22.

hazbo commented 8 years ago

The pull request I've just made should fix this bug, @jishi9 does this look reasonable to you?

hazbo commented 8 years ago

I've just ran through this process a couple of times since fixing this bug, and it also appears to change the status of the review as a whole.

The discussion we had in that last thread outlined that this wasn't the case. Since changing this code, it now is. At this stage I'm not sure whether or not that is intended. But yeah - just thought I'd mention that.

barbastan commented 8 years ago

I am eager to hear what Omar originally had in mind.

Thanks again. Barbara

On Wednesday, December 23, 2015, Harry Lawrence notifications@github.com wrote:

I've just ran through this process https://github.com/google/git-appraise/issues/17#issuecomment-166888010 a couple of times since fixing this bug, and it also appears to change the status of the review as a whole.

The discussion we had in that last thread outlined that this wasn't the case. Since changing that code, it now is. At this stage I'm not sure whether or not that is intended. But yeah - just thought I'd mention that.

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/issues/22#issuecomment-166945371.

jishi9 commented 8 years ago

@hazbo

It also appears to change the status of the review as a whole. The discussion we had in that last thread outlined that this wasn't the case. Since changing this code, it now is.

I don't think that's the case, the status of review as a whole was already displayed fine. As far as I could tell the bug only pertained to how the status of individual commits are displayed (always fyi, as opposed to lgtm or needs work where appropriate). Your change does fix this problem.

Looking at review.go where the CommenThread.Resolved field is defined, I'm thinking that maybe the original implementation of show was correct in using thread.Resolved rather than comment.Resolve. The bug might be that the value of thread.Resolved is not updated beforehand using updateResolvedStatus. I have created pull request #24 to fix this.

With this change things started to make more sense. This is the model I have pieced out:

A review's overall status:

A comment-thread's overall status (this is a recursive definition):

N.B. A comment-thread may be a global, attached to a file, or attached to a line.

ojarjur commented 8 years ago

@jishi9 Yes, your interpretation is exactly what was intended (and your bug fix was a good one). I merged your PR, and it seems to have fixed the bug (from what I've seen) so I'm going to close this issue.

Please re-open it if you find anything seems off.

Also, thanks for digging into this and sending out the fix!