The show command does not display comment status correctly

[jishi9]

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]

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
#22.

[hazbo]

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

[hazbo]

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]

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
#17 (comment)
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
#22 (comment).

[jishi9]

@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:

• There are no comment-threads, or all comment-thread statuses are FYI => pending
• There is at least one LGTM comment-thread, and no NMW comment-threads => accepted
• There is at least one NMW comment-thread => rejected

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

• root comment is NMW
  • there exists a direct LGTM child && no direct NMW child exists => FYI
  • otherwise => NMW
• root comment is LGTM
  • direct NMW child exists => NMW
  • otherwise => LGTM
• root comment is FYI
  • direct NMW child exists => NMW
  • otherwise => FYI

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

[ojarjur]

@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!