search

jordanlewis / re

re is a ✨CLI ✨code review tool for GitHub. Do code reviews from the comfort of your $EDITOR!
Apache License 2.0
53 stars 4 forks source link

Doesn't really work for PRs on branches with unsquashed commits #4

Open rmloveland opened 5 years ago

rmloveland commented 5 years ago

If a PR has several commits on the branch, re shows a diff that does not reflect all of the changes that make up the PR, but only a subset (the latest commit?).

This may be how git works and have nothing to do with re. But if there is a way for re to work in this instance, that would be cool.

Otherwise I volunteer to update the README to note that you must be working with PRs on branches that have all their commits squashed (which seems like the right way to live anyway - just surprised me in this case).

jordanlewis commented 5 years ago

Hmm - that doesn't seem right. Can you give me an example of a PR that has this problem? re was designed specifically for multi-commit PRs.

rmloveland commented 5 years ago

Probably too late to be useful now, but it was this docs PR. I ended up asking Lauren to squash so I could do the review with re (which worked fine at that point).

I did find the re-edit-* file from the earlier attempt. I'm attaching it to this comment. Hopefully it shows you what was happening (it's possible this is a PEBKAC situation).

re-edit-201726569.txt

If that is not helpful I will set up another PR to try to repro.

jordanlewis commented 5 years ago

Ah - yes! That file actually seems correct to me. re presents a review as one set of diffs per commit. If you scroll through that file, you can see that all of the diffs are eventually available for each file - they're just not squashed for you. This allows you to leave comments on diffs from multiple different commits, which facilitates a mode of PR-review where people are trying to PR a sequence of fully-baked commits at once.

The current mode of operation of re isn't set up to facilitate review of PRs that have multiple "fixup" commits stacked onto each other - for that you do want a squash mode of review, which I haven't implemented yet. But perhaps we can add that as an option - I don't think it's too hard to look at the full-PR diff from GitHub's API instead of the per-commit diff. In fact, implementing the per-commit diff was one of the more difficult and annoying tasks of implementing re.

On Mon, Sep 10, 2018 at 5:27 PM Rich Loveland notifications@github.com wrote:

Probably too late to be useful now, but it was this docs PR https://github.com/cockroachdb/docs/pull/3669. I ended up asking Lauren to squash so I could do the review with re (which worked fine at that point).

I did find the re-edit-* file from the earlier attempt. I'm attaching it to this comment. Hopefully it shows you what was happening (it's possible this is a PEBKAC situation).

re-edit-201726569.txt https://github.com/jordanlewis/re/files/2368452/re-edit-201726569.txt

If that is not helpful I will set up another PR to try to repro.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/jordanlewis/re/issues/4#issuecomment-420067205, or mute the thread https://github.com/notifications/unsubscribe-auth/AACrLVLUiFRsu1qGrIEcJMbLTK5P9qcIks5uZtlZgaJpZM4WiKgG .

rmloveland commented 5 years ago

This issue appears to have been user confusion on my part and can be closed IMO. Just filed a feature request for the "squash mode" in #5.