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

New wordwrap #9649

Closed
wants to merge 4 commits into from
Closed

New wordwrap #9649

wants to merge 4 commits into from

Conversation

krux02
Copy link
Contributor

@krux02 krux02 commented Nov 8, 2018

this is supposed to fix #3196. I have a question about the name though. The function does more than just wrapping words, it fills up the lines so that you don't end up with lines with just a single word in them when a newline is inserted. In emacs uses function names such as fill-paragraph and fill-region.

proc `$`*(rune: Rune): string =
## Converts a Rune to a string
rune.toUTF8

proc `$`*(runes: seq[Rune]): string =
## Converts a sequence of Runes to a string
result = ""
for rune in runes: result.add(rune.toUTF8)
for rune in runes:
result.add rune
Copy link
Member

Choose a reason for hiding this comment

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

Excellent!


proc wordWrap*(s: string, maxLineWidth = 80,
splitLongWords = true,
newLine = "\n"): string =
Copy link
Member

Choose a reason for hiding this comment

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

two spaces before the '='

## whitespace is treated equally. Non-breaking whitespace is
## ignored.

var currentWordLength: int = 0
Copy link
Member

Choose a reason for hiding this comment

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

Use type inference, ': int' is annoying.

## ignored.

var currentWordLength: int = 0
var currentWord: string = newStringOfCap(32)
Copy link
Member

Choose a reason for hiding this comment

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

Likewise for these.

var currentWordLengthAtLineEnd: int = -1
var longWordMode = false

template handleWhitespace(): untyped =
Copy link
Member

Choose a reason for hiding this comment

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

that should be better named addWord.

currentLineLength = 0

if currentLineLength > 0:
result.add ' '
Copy link
Member

Choose a reason for hiding this comment

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

Well, don't nuke the whitespace replacing it by a single space.

@@ -0,0 +1,67 @@
import unicode

proc wordWrap*(s: string, maxLineWidth = 80,
Copy link
Member

Choose a reason for hiding this comment

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

procs use verbs, so the name should be wrapWords.

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't make sense. "wordwrap" can be a verb too. Using your argument, we'd then have "echoes", "aligns", "adds", etc.

Sorry, I went bonkers because I thought you suggested "wordwraps" instead of "wrapwords".

I agree that "wrapwords" is better (not because of thr verb argument; wordwraps can be read as a verb too), but because of correctness.. we are wrapping many "words" across lines, not just a single word.

if currentWord.len > 0:

if currentLineLength + 1 + currentWordLength > maxLineWidth:
result.add newLine
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
result.add newLine
if len(result) > 0:
result.add newLine

otherwise you are inserting an empty line, if the length of the first word on the first line is >= maxLineWidth.

@skilchen
Copy link
Contributor

skilchen commented Nov 8, 2018

This wordWrap does something other than strutils.wordWrap.

wordwrap.wordWrap("abc\ndef") == "abc def"

while

strutils.wordWrap("abc\ndef") == "abc\ndef"

is this intentional?

@krux02
Copy link
Contributor Author

krux02 commented Nov 8, 2018

@skilchen for me this change is intentional, but it seems Araq disagrees on it.

@Araq
Copy link
Member

Araq commented Nov 8, 2018

Did it my way based on your work.

@Araq Araq closed this Nov 8, 2018
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.

Is there a way to word wrap UTF-8?
4 participants