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

Always treat ranges as exclusive in Range.contains?/2 #763

Closed
wants to merge 2 commits into from
Closed

Always treat ranges as exclusive in Range.contains?/2 #763

wants to merge 2 commits into from

Conversation

zachallaun
Copy link
Collaborator

Range.contains?/2 was treating the end-position of ranges as inclusive for single-line ranges, but exclusive for multi-line ranges. Our ranges should always be exclusive of the end character, however, so this was a bug. There were two other bugs/compensations that were counteracting it (at least in some cases), however:

  1. Cursor positions in tests

There were a lot of tests with cursor positions like this:

foo| = :foo

However, this cursor is actually "pointing" to the space after foo, not to the last character of foo:

foo = :foo
#  ^

Fixing Range.contains?/2 caused all of the tests with those kinds of cursor positions to break, but I believe those tests were incorrect in the first place. I changed those cursor positions to point to the first character of the entity instead (|foo).

  1. String ranges with a negative end offset

When detecting in strings, a number of fetch_range calls used a -1 end offset. This counteracted the inclusiveness of the end character in Range.contains?, but once that was fixed, string ranges became incorrect (short one character).

@zachallaun
Copy link
Collaborator Author

Closing in favor of #768

@zachallaun zachallaun closed this Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant