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

Dripping #963

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

Dripping #963

wants to merge 1 commit into from

Conversation

gbebgdee
Copy link

@gbebgdee gbebgdee commented Jul 9, 2021

No description provided.

@gbebgdee gbebgdee marked this pull request as draft July 9, 2021 03:55
@gbebgdee gbebgdee marked this pull request as ready for review July 9, 2021 03:55
@balthisar
Copy link
Member

I like it (see #519), but can you clean up the commit a little bit? We shouldn't leave the console application with the callback code activated, for example, as well as the other commented out code.

More importantly, are you certain that the generic approach works for EVERY existing string? A quick scan doesn't look like there would be any issues. One concern, though, is if you take a look at e.g., TC_TXT_HELP_3, by swallowing the newlines, you're also swallowing the vertical spacing. Maybe it's not a concern.

I'm looking at this quickly, but if a string ends with two newlines, are you going to overrun the string that you're copying?

I wonder if it's not time to finally just remove the newlines from the source, and perform wrapping in the console application. This has been done and rejected before.

Let me know your thoughts.

Whatever the case, I may defer this a couple of weeks in order to issue a stable 5.8 release with strings as they are, and make this the first commit on 5.9.

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

2 participants