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

TerminalLink endIndex should be inclusive (and it should say that in the documentation) #101394

Closed
alexr00 opened this issue Jun 30, 2020 · 4 comments
Assignees
Labels
api terminal General terminal issues that don't fall under another label terminal-links under-discussion Issue is under discussion for relevance, priority, approach

Comments

@alexr00
Copy link
Member

alexr00 commented Jun 30, 2020

Testing #101300

The end position of a range used for a DocumentLink appears to be inclusive. I would expect the endIndex of a TerminalLink to behave the same way. Same code to calculate end position results in different range being used:

image

@alexr00 alexr00 changed the title TerminalLink endIndex should be inclusive (and is should say that in the documentation) TerminalLink endIndex should be inclusive (and it should say that in the documentation) Jun 30, 2020
@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug terminal General terminal issues that don't fall under another label labels Jun 30, 2020
@Tyriar Tyriar added this to the June 2020 milestone Jun 30, 2020
@Tyriar
Copy link
Member

Tyriar commented Jun 30, 2020

InputBoxOptions.valueSelection uses exclusive end index, so does String.prototype.substring. Opinions @jrieken?

@jrieken
Copy link
Member

jrieken commented Jul 1, 2020

Yeah, given these are two indexes into a "flat" string I'd stick with substring and valueSelection logic. I see the inconsistency, we can argue that one is ranges with position objects and the other is offsets...

@alexr00
Copy link
Member Author

alexr00 commented Jul 1, 2020

How often do you expect people to want to share code between a document link provider and a terminal link provider? The inconsistency is annoying in that case because you have to have an extra +1 or -1 somewhere, and that feels broken.

@Tyriar Tyriar added under-discussion Issue is under discussion for relevance, priority, approach and removed bug Issue identified by VS Code Team member as probable bug labels Jul 1, 2020
@Tyriar Tyriar modified the milestones: June 2020, July 2020 Jul 1, 2020
@Tyriar
Copy link
Member

Tyriar commented Jul 14, 2020

Decided not to do this #91290 (comment)

@Tyriar Tyriar closed this as completed Jul 14, 2020
@Tyriar Tyriar removed this from the July 2020 milestone Jul 14, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Aug 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api terminal General terminal issues that don't fall under another label terminal-links under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

3 participants