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

Fix reflow for CRLF line ending (wip) #2672

Closed
wants to merge 2 commits into from
Closed

Conversation

koiuo
Copy link

@koiuo koiuo commented Jun 4, 2022

This is a fix for the #2645.

It assumes a fix for the textwrap library, which is already in master but not yet released.

See details here mgeisler/textwrap#453

Here's a quick demonstration of how it works
https://asciinema.org/a/KZNzsnuHYXppwzw6tvVHyfiSe

@koiuo
Copy link
Author

koiuo commented Jun 4, 2022

@kirawi , could you please take a look and let me know if overall approach looks ok?
Thanks for your time.

@koiuo koiuo marked this pull request as ready for review July 10, 2022 11:00
@the-mikedavis
Copy link
Member

This looks good besides the clippy lint. Looks like your PR was merged but isn't released in a version of textwrap yet? mgeisler/textwrap#454

@koiuo
Copy link
Author

koiuo commented Aug 6, 2022

@the-mikedavis , I fixed the clippy error.

Yes, the change in textwrap is not yet released.
Are you are fine with this PR overall? If you intend to merge some time in the future I'll ask textwrap maintainer for an ETA on their side. If you want to merge asap, I'll ask to cut a release.

@the-mikedavis
Copy link
Member

Yeah this is looking good to me 👍. No need to rush I don't think, it'd be good to have at least one other reviewer check this out before merge anyways

@kirawi kirawi added C-bug Category: This is a bug A-dependencies Area: Dependency upstream S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 13, 2022
@mgeisler
Copy link

Yes, the change in textwrap is not yet released.

There has been a Textwrap release now, you should be able to depend on 0.16.0 and get the line-ending control you need.

@pascalkuthe
Copy link
Member

I am not sure is this is the right approach. It fixes the common case of CRLF linenedings but actually just exposes a more deeply rooted issue: text-wrap handles line breaks differently then helix. In fact this PR will cause new bugs for crlf files which also contain LF linenedings which are not detected as line breaks at all anymore with this PR. It also fails for other supported (unicode) line-ending and CR line endings.

AFAIK the line_ending field on Document is just the line break that is used for inserting new line but the document may contain other linebreaks. I wish text-wrap would just let us supply our own lines iterator. We could use ropeys lines iterator here which correctly accounts for the linebreak feature flags helix uses. We could easily strip the EOL character ourselfs.

@pascalkuthe
Copy link
Member

I am closing this pr as it has gone stale (over a year without development). Like I said in a different issue we are planning to move away from textwrap and would instead reuese our own softwrap infrastructure in the long run to gain the flexibility to solve issues like this.

Thank you for contributing!

@pascalkuthe pascalkuthe closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency C-bug Category: This is a bug S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. upstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants