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

Test Rope.getLine #4303

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Test Rope.getLine #4303

wants to merge 2 commits into from

Conversation

jhrcek
Copy link
Collaborator

@jhrcek jhrcek commented Jun 9, 2024

Just testing out newly introduced utility from text-rope

@michaelpj
Copy link
Collaborator

We also do this a bunch in lsp, I think.

@jhrcek
Copy link
Collaborator Author

jhrcek commented Jun 9, 2024

We also do this a bunch in lsp, I think.

@michaelpj Hmm, in lsp there's some extractLine
in LSP which returns a Rope and does further manipulation on that Rope, which means that the new function I introduced is probably useless. Or do you think that the function I introduced should return a Rope?

@michaelpj
Copy link
Collaborator

Hmm, that's a little annoying. We return a Rope so we can use the mixed encoding indexing which text-rope provides, but conceptually it's just a one-line Text. I don't know if there are comparable mixed-indexing functions for plain Text? If so we can switch to use your getLine.

@michaelpj
Copy link
Collaborator

Note that it's quite likely that functions in HLS also need this facility. Remember that much of HLS incorrectly assumes that it can use LSP positions as GHC positions or as indexes into Text. So e.g. this line is wrong: it needs to drop c UTF-16 code units, whereas Text.drop drops code units.

So HLS might also prefer to get a Rope. Or it should use rangeLinesFromVFS from lsp, which does this correctly.

@jhrcek
Copy link
Collaborator Author

jhrcek commented Jun 11, 2024

@michaelpj I opened a followup PR in text-rope that makes getLine "stay in the Rope world". Would it make more sense to you like that? Bodigrim/text-rope#7

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.

2 participants