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

[easy] Disable terminal text wrapping if message contains URL #133

Merged
merged 2 commits into from
Sep 7, 2022

Conversation

keyz
Copy link
Contributor

@keyz keyz commented Aug 24, 2022

Summary

Minor issue: by default textwrap can break a URL into multiple lines. This makes PR links unclickable when the terminal window is small.

I played with a few config options on https://mgeisler.github.io/textwrap/ — think this is the best combo?

image

Test Plan

Before After
Screen Shot 2022-08-23 at 9 44 16 PM Screen Shot 2022-08-23 at 9 45 12 PM

Comment on lines 21 to 25
if lazy_regex::regex_is_match!(r#"https?://"#, text) {
options.break_words = false;
options.word_separator = textwrap::WordSeparator::AsciiSpace;
options.word_splitter = textwrap::WordSplitter::NoHyphenation;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These options look reasonable. I'd be okay with using them unconditionally.

Suggested change
if lazy_regex::regex_is_match!(r#"https?://"#, text) {
options.break_words = false;
options.word_separator = textwrap::WordSeparator::AsciiSpace;
options.word_splitter = textwrap::WordSplitter::NoHyphenation;
}
options.break_words = false;
options.word_separator = textwrap::WordSeparator::AsciiSpace;
options.word_splitter = textwrap::WordSplitter::NoHyphenation;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good! just updated the diff

Copy link
Contributor

@sven-of-cord sven-of-cord left a comment

Choose a reason for hiding this comment

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

Nice!

Thanks for doing this, and sorry for my slow initial response. I only came back from holiday last Thursday.

@sven-of-cord sven-of-cord merged commit df5fe6e into spacedentist:master Sep 7, 2022
clarityflowers pushed a commit to clarityflowers/spr that referenced this pull request Mar 19, 2024
### Summary

Minor issue: by default `textwrap` can break a URL into multiple lines. This makes PR links unclickable when the terminal window is small.

I played with a few config options on https://mgeisler.github.io/textwrap/ — think this is the best combo?

<img width="600" alt="image" src="https://user-images.githubusercontent.com/2268452/186331956-5b183ff6-6b3f-4ac3-82e8-72d2044abda8.png">


### Test Plan

| Before  | After |
| ------------- | ------------- |
| <img width="491" alt="Screen Shot 2022-08-23 at 9 44 16 PM" src="https://user-images.githubusercontent.com/2268452/186331402-0943f667-6a9f-4c2f-8961-b12997c12b6b.png"> | <img width="491" alt="Screen Shot 2022-08-23 at 9 45 12 PM" src="https://user-images.githubusercontent.com/2268452/186331444-984aeecb-30df-4f59-b84b-0a7735d80bfb.png"> |

Pull Request: spacedentist#133
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