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

Refactor getLine to return TextLines/Rope #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhrcek
Copy link
Contributor

@jhrcek jhrcek commented Jun 11, 2024

As @michaelpj pointed out, for many use-cases it might be preferable if getLine stays within the world of Ropes, to allow for further processing (an example of this is in lsp and similar, currently incorrect usage of getLine-like functionality mentioned in this comment in hls: haskell/haskell-language-server#4303 (comment)

The initial impl. ini this PR might not be ideal, because its complexity is linear in the length of line being wrapped. But let me open a PR to get initial feedback.

@Bodigrim
Copy link
Owner

It feels a bit strange when lines gives [Text], but getLine returns TextLines / Rope. Does it save much on HLS side? One can apply fromText there, probably to the same effect.

@michaelpj
Copy link

That would be fine I guess. Bascially we need the mixed-encoding indexing that we get from Text.Rope.Mixed! It feels a little strange to be converting back to Text and then back to Rope, but maybe that's fine?

getLine lineIdx rp =
case T.unsnoc firstLine of
fromText $ case T.unsnoc firstLine of
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to return Rope, can we avoid firstLineRope and stick to firstLineRope? Otherwise we are doing a double conversion.

@michaelpj
Copy link

I guess the neatest alternative would be to provide some utilities for mixed-encoding indexing of Text itself, on the reasoning that it should be okay on the Texts we get from getLine, since they are likely to be short. WDYT @Bodigrim ?

@Bodigrim
Copy link
Owner

After thinking about it, I'm largely fine with both options:

  • We keep functions as they were before this PR, TextLines -> Text, Rope -> Text, and HLS wraps results in fromText to get access to encoding-based indexing. The only downside I see is slight inefficiency: e. g., a hypothetical getLine :: Int -> TextLines -> TextLines knows that the result contains no newlines, while with fromText . (getLine :: Int -> TextLines -> Text) we would have scan Text once again. Not a big deal for the sort of strings HLS usually operates on.
  • We change function signatures as in this PR. I'm fine to merge it, if HLS finds it useful. The only requirement is to be efficient and avoid double conversion. E. g., getLine :: Int -> Rope -> Rope should not convert intermediate Rope to Text only to execute fromText later.

@jhrcek
Copy link
Contributor Author

jhrcek commented Jun 14, 2024

I'll get to it over weekend. Will probably go with option 2 (keep Rope/TextLines).
Regarding avoiding double conversion I'll just have to figure out how to drop the last \n from the Rope without converting it to Text (or alternatively throw away/change the nice looking property that Rope.getLine i r == Text.lines (Rope.toText r) !! i. Suggestions are welcome 😄

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.

None yet

3 participants