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.

Sign up for GitHub

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

Jump to bottom

Maybe a mistake fix #52

Closed
wants to merge 1 commit into from
Closed

Maybe a mistake fix #52

wants to merge 1 commit into from

Conversation

Veetaha
Copy link

@Veetaha Veetaha commented Feb 22, 2020

?

@Veetaha Veetaha requested a review from matklad February 22, 2020 17:10
@CAD97
Copy link
Collaborator

CAD97 commented Feb 22, 2020

I don't think this is wrong. When we receive an offset equivalent to the length of the selected span, the correct token to say is present there is None, as is handled for the empty span case directly below the assert.

An actual example (other than an empty span) that passes the assert on master and is caught by this patch would help in determining the correct/expected behavior here.

@Veetaha
Copy link
Author

Veetaha commented Feb 22, 2020 •
edited
Loading

Oh, I just read the precondition comment and saw the assertion that kinda conflicts with it...

@matklad
Copy link
Member

matklad commented Feb 25, 2020

Yep, text offset can point right at the end of the node/file, it can be equal to the length of the thing.

@matklad matklad closed this Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matklad matklad Awaiting requested review from matklad

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Veetaha @CAD97 @matklad