Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Conversation

@jcansdale
Copy link
Collaborator

This PR matches diff lines based on their original line number and content.

  • Diff lines can be matched based on a single line (multiple lines of context aren't required)
  • Lines can be inserted between and near to comments without them vanishing

Fixes #1052

@jcansdale jcansdale requested a review from grokys July 13, 2017 11:35
@grokys
Copy link
Contributor

grokys commented Jul 13, 2017

There are problems with this approach. For example, I created this PR: grokys/PullRequestSandbox#29

As you can see, I added a comment on line 22, but in VS with this PR, this is showing up on line 15:

image

@jcansdale
Copy link
Collaborator Author

jcansdale commented Jul 13, 2017

@grokys Well spotted. 😄 I've been wondering about how to make it more robust. Could you try this latest push. It will match using all available context lines before falling back to matching using the original line number.

Still not perfect, but I wonder if this will end up being about getting the balance right (false positives vs false negatives).

@grokys
Copy link
Contributor

grokys commented Jul 13, 2017

It now shows the unedited file with the comments in the correct place, but when I delete the Guard.NotNull as the comment suggests, the comment jumps to line 15 again.

@jcansdale
Copy link
Collaborator Author

jcansdale commented Jul 13, 2017

From @grokys on the Slack:

i think something like that will give much better results
but obviously, it's more involved which is why i was punting it until we've got the basic stuff working...
tbh i think the only _real_ way to do it would be to track the position of the comments as edits are made and store them locally
then use an algorithm like the above to match comments if the file is e.g. edited outside VS
we also need a "outdated" visual state to show that the comment may no longer be valid
to me it feels like a separate minor release

I'm going to push a version that fixes the original issue. It was trying to match 5 lines of context when the diff only had 3.

@jcansdale jcansdale force-pushed the fixes/1052-added-comment-not-found branch from 09b044f to af9dfaa Compare July 13, 2017 14:04
@jcansdale
Copy link
Collaborator Author

I've just pushed more of a point fix that should resolve just this issue. Hopefully 3rd time's a charm? 😉

Copy link
Contributor

@grokys grokys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jcansdale jcansdale merged commit 21c38f1 into master Jul 13, 2017
@jcansdale jcansdale deleted the fixes/1052-added-comment-not-found branch July 13, 2017 18:02
@jcansdale
Copy link
Collaborator Author

I've done some more testing on .com this seems to behave very similarly. Sometimes .com requires less than 5 lines of context, but when I noticed this the API returned only 4 lines so the extension actually behaved in a similar way.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants