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

The n/p keys should obey "Include changes in files previously reviewed …"

Closed
lilyball opened this issue Oct 4, 2016 · 6 comments
Closed

Comments

@lilyball
Copy link

lilyball commented Oct 4, 2016

When multiple people are doing independent reviews of the same PR, only the first reviewer can actually use n/p, because those shortcuts skip files that were reviewed by other people. This is quite annoying. I think these keys should obey the "Include changes in files previously reviewed only by others" checkbox and, if checked, should only skip files that you yourself have marked as reviewed.

@pkaminski
Copy link
Member

pkaminski commented Oct 7, 2016

Sorry for the delay; I needed to think about this one a bit and was busy with other stuff for a while.

The n/p shortcuts do actually pay attention to the flag, but the flag's meaning is subtly different from what you expect. Toggling on "Include changes in files previously reviewed only by others" means "first check if a file is considered reviewed, and if it's not then don't pay attention to who reviewed it in the past -- it's in scope". Notice that checking for past reviewers only takes place after it's determined that the file is unreviewed (at the latest revision).

The good news is that you control whether a file is considered reviewed or not using the custom review completion condition and the files[...].reviewed flag. You could, for example, require at least two distinct review marks, or a review mark from every participant in the review. You could even just always set it to false for all files, in which case they'd always show up as red for everyone unless they had personally reviewed them; the downside is that the grey unreviewed file counter would never go to zero in your reviews. But overall review completion is independent of the nominal review state of each file (for customized conditions), so it wouldn't be affected.

Thoughts?

@lilyball
Copy link
Author

lilyball commented Oct 7, 2016

I didn't realize the custom review completion conditions could actually control the red/green marks on individual files.

In any case, not having the gray mark go down would be annoying. The overwhelming majority of our PRs are only ever reviewed by one person. My request here is most useful in the case where one person starts reviewing a huge PR and on gets halfway, then a second person comes along to review the thing (or in the rare case where a PR is complicated enough that the reviewer wants a second opinion). I don't want to penalize all reviews just to make n/p work better on these occasional doubly-reviewed PRs.

@pkaminski
Copy link
Member

pkaminski commented Oct 7, 2016

These are starting to become pretty specialized use cases:

  1. Usually in a handover you don't want to re-review diffs that the first person already signed off on.
  2. For occasional "second opinion" situations you probably want to codify that in your custom condition, e.g. by requiring two reviewers iff a special label is set. This would actually make the unreviewed file count useful in that case.

I could also add a set of bindable "next file unreviewed by me" functions -- easy enough, but a bit of a hack IMO.

@lilyball
Copy link
Author

lilyball commented Oct 7, 2016

I think the confusion here is that marking a file is reviewed has two completely separate meanings. The first meaning is the reviewer has seen that state of the file and wants any future reviews by them to only look at subsequent diffs. The second meaning is actually "signing off" on the file as valid. We only care about the first meaning. We have a custom review completion condition set up to require :lgtm: to actually sign off on the review as a whole, but there's no such thing as signing off on individual files. And even if there were, a second reviewer would want to look over those files anyway so that way they actually know what's in them; it's hard to review only half of a PR and be confident you understand what the PR is doing, if you didn't actually look at the other half of the diff.

@lilyball
Copy link
Author

lilyball commented Oct 7, 2016

That said, a set of bindable "next file unreviewed by me" functions would satisfy my needs.

@pkaminski
Copy link
Member

pkaminski commented Oct 7, 2016

Extra bindings now available (nextPersonallyUnreviewedFile(), etc.).

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

No branches or pull requests

2 participants