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

Add getLine to get line by index #6

Merged
merged 6 commits into from
Jun 9, 2024
Merged

Conversation

jhrcek
Copy link
Contributor

@jhrcek jhrcek commented May 1, 2024

WIP - implementing #5
It's a bit more labor intensive than I initially anticipated, so opening draft for potential early feedback.

TODO:

  • add getLine for TextLines
  • add getLine to Rope(s)
  • add getRange for TextLines
  • add getRange for Rope(s)

@jhrcek
Copy link
Contributor Author

jhrcek commented May 4, 2024

Hello @Bodigrim ,
can I get some preliminary feedback on the first of the functions I intended to implement when you have some time to spare?
I'll get to the Range variant in the next few days..

@@ -562,3 +563,21 @@ utf16SplitAtPosition (Utf16.Position l c) rp = do
let (beforeLine, afterLine) = splitAtLine l rp
(beforeColumn, afterColumn) <- utf16SplitAt c afterLine
Just (beforeLine <> beforeColumn, afterColumn)

-- | Get a line by its 0-based index.
-- Returns "" if the index is out of bounds.
Copy link
Owner

Choose a reason for hiding this comment

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

I think monospace font would look better, more prominent:

Suggested change
-- Returns "" if the index is out of bounds.
-- Returns @""@ if the index is out of bounds.

src/Data/Text/Rope.hs Outdated Show resolved Hide resolved
Copy link
Owner

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

Overall makes sense to me, thanks for working on it.

@jhrcek
Copy link
Contributor Author

jhrcek commented May 6, 2024

Thanks for review.
I'll still implement the getRange helper and try to test out the new functions in hls before marking it as ready to merge.

@Bodigrim Bodigrim mentioned this pull request Jun 8, 2024
@Bodigrim
Copy link
Owner

Bodigrim commented Jun 8, 2024

@jhrcek how is it going? Any plans for further progress?
I plan to make a release sooner than later, so it would be great to merge your work once complete.

@jhrcek
Copy link
Contributor Author

jhrcek commented Jun 8, 2024

Hello @Bodigrim
I saw you today at ZuriHac. I'll try to catch you tomorrow in person and we can discuss the next steps. I'd like to gather some more feedback from other HLS devs to see if it's the right direction and we could potentially merge this.

@Bodigrim
Copy link
Owner

Bodigrim commented Jun 8, 2024

@jhrcek sure, let's catch up tomorrow. Or today if it still works for you.

Copy link

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Looks good to me.

src/Data/Text/Lines/Internal.hs Outdated Show resolved Hide resolved
@jhrcek jhrcek changed the title Add getLine for TextLines Add getLine to get line by index Jun 9, 2024
@Bodigrim Bodigrim marked this pull request as ready for review June 9, 2024 10:32
@Bodigrim Bodigrim merged commit fe8583c into Bodigrim:master Jun 9, 2024
13 checks passed
@Bodigrim
Copy link
Owner

Bodigrim commented Jun 9, 2024

Thanks a lot @jhrcek!

@Bodigrim
Copy link
Owner

Bodigrim commented Jun 9, 2024

@jhrcek the repo is in ready-for-release state now, waiting for you to green-light that it fits HLS needs.

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.

3 participants