Skip to content

Fix suggestions algorithm #110

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

Merged
merged 7 commits into from
Jan 21, 2025
Merged

Fix suggestions algorithm #110

merged 7 commits into from
Jan 21, 2025

Conversation

pbrisbin
Copy link
Member

@pbrisbin pbrisbin commented Jan 21, 2025

At its core, getting suggestions from a patch involves looking for contiguous blocks of
either removes, or removes-then-adds. The former would be a suggestion to remove
some content, and the latter would be a suggestion to replace it.

The old version of this algorithm used line numbers to look for contiguous blocks: if
a line's number was the last line + 1, it was considered contiguous. This is wildly
incorrect since removed and added lines' numbers increment independently.

This was working fine, which made all our tests pass:

1 1 # a line
2   # remove this
  2 # add this

But this example breaks:

1 1 # a line
2   # remove this
3 2
4 3 # more
5   # remove this
  4 # add this

Notice how 4 (on the add side) should be considered as contiguous with 5 (on the
remove side). Obviously, looking for line numbers based on previous line numbers
won't work.

The new algorithm groups lines of equal type (add, remove, context)1 and also
considers an add immediately following a remove as equal. This turns a diff like
the above into the kind of groupings we'd visualize when looking for suggestions,

---
1 1 # a line
---
2   # remove this
---
3 2
4 3 # more
---
5   # remove this
  4 # add this
---

Once grouped, any group that has removes is a suggestion. If it also has adds, that's used
otherwise it's a suggested removal.

Footnotes

  1. This, sadly, meant basically re-implementing parse-git-patch ourselves.

- Add/fix the failing hyphen-deletion test case
- Use discriminated union, so we can include context
- Rename types
Since line numbers aren't monotonic on either side, we really have to
implement something that matches how one *visually* sees the
suggestions. We can do this by grouping the lines and then looking for
groups of removes or removes-then-adds, which can be turned into
suggestions.
This fails for large PRs. Instead, we can use our existing `/files`
response and parse it's `patch` attribute.
@pbrisbin pbrisbin changed the title pb/group by Fix suggestions algorithm Jan 21, 2025
@pbrisbin pbrisbin marked this pull request as ready for review January 21, 2025 20:59
@pbrisbin pbrisbin added the bug Something isn't working label Jan 21, 2025
@pbrisbin pbrisbin merged commit a71ed04 into main Jan 21, 2025
4 checks passed
@pbrisbin pbrisbin deleted the pb/group-by branch January 21, 2025 20:59
@pbrisbin pbrisbin added bugfix and removed bug Something isn't working labels Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant