WASasquatch / was-node-suite-comfyui

An extensive node suite for ComfyUI with over 210 new nodes
MIT License
1.15k stars 170 forks source link

[BUG] WAS_Text_Multiline doesn't process lines properly #304

Closed Arcitec closed 9 months ago

Arcitec commented 9 months ago

Relevant code:

        new_text = []
        for line in io.StringIO(text):
            if not line.strip().startswith('#'):
                if not line.strip().startswith("\n"):
                    line = line.replace("\n", '')
                new_text.append(line)
        new_text = "\n".join(new_text)

The issues are as follows:

  1. Stripping repeatedly is wasteful.
  2. Stripping removes all whitespace and line endings, so the if not line.strip().startswith("\n") is literally False 100% of the time no matter the input. So if someone inputs a bunch of blank line in the box, the output will be \n\n\n\n\n\n even though you wanted to avoid exactly that.
  3. line = line.replace("\n", '') is kinda nonsensical. There can only be one newline per line. It's more appropriate to rstrip() to clean all trailing whitespace/newlines.

Fixed code would look something like this:

        new_text = []
        for line in io.StringIO(text):
            line_stripped = line.strip()
            if not line_stripped == "":
                if not line_stripped.startswith('#'):
                    line = line.rstrip()
                    new_text.append(line)
        new_text = "\n".join(new_text)
WASasquatch commented 9 months ago

You do realize the newline isn't necessarily \n physically in the text right? \n could come from text file while newlines entered in a node doesn't create a \n in raw string similar to using '''entering linebreaks proxies no \n'''

The input isn't necessarily always coming from the input field, but could be a input connection from something else. Additionally they could write a \n out manually as was an original problem.

A bug usually constitutes a problem with its usage. Does this cause any problems with normal usage? I haven't seen any? But does fix bugged usage or ulterior formats.

Arcitec commented 9 months ago

What? What you just said literally makes no sense at all and is just completely wrong. :) If you're interested in replacing manually written "\n" (where someone manually wrote "hello\nthere" in the text box in ComfyUI), then nothing in your current code does that!

A manual, escaped/"not real" newline looks like this in Python: "\\n"

Your current code attempts to replace "\n" which is a real newline. It does nothing at all for people's manually written "\n" (backslash n) character sequences.

Anyway. It doesn't matter whatsoever what the input to your node is. Everything I said is accurate for all input. The io.StringIO(text) splits the input data stream on every real newline, meaning that line only ever contains 1 line each time, and there's only one newline per line (at the end of each line it splits).

Your current code never even runs the line = line.replace("\n", '') line at all. Ever. It's impossible to reach that line due to the buggy if-statement above it.

The fixed code I suggested takes care of all of those bugs.

But we don't have to agree about this. I already wrote my own nodes instead since these are too buggy.

Arcitec commented 9 months ago

Here's a different summary of the bugs, perhaps it's easier to see.

        new_text = []
        for line in io.StringIO(text):
            if not line.strip().startswith('#'):
                if not line.strip().startswith("\n"): <--- ALWAYS false, 100% of the time, no matter what input. And wasteful extra strip() call.
                    line = line.replace("\n", '') <--- NEVER runs, EVER. And it's better to use .rstrip() to actually remove trailing whitespace/newlines.
                new_text.append(line)
        new_text = "\n".join(new_text)

So your current code does this:

        new_text = []
        for line in io.StringIO(text):
            if not line.strip().startswith('#'):
                new_text.append(line)
        new_text = "\n".join(new_text)

That's all it does. Nothing else. The other lines literally never run, ever.

PS: I also see now that io.StringIO() strips the trailing newlines already. The variable line never contains any trailing newline, so there's no reason to check for "\n" since it will never exist inside line. But that's just extra icing on top of the fact that you already did .strip() before checking for "\n" which is already nonsensical, since .strip() deletes every leading/trailing "\n" from the string already, so that if-statement would never be true anyway.

To fix all of those issues, the proposed code in the first comment would work.

WASasquatch commented 9 months ago

Where is the bug, though? You're really just knit picking over redundancy on one nodes code in a file full of stuff needing basically complete rewrites. Lol Maybe just make a PR.

Notice the update on readme, the project is inactive. So unless you are PRint it ain't going change any time soon.

Arcitec commented 9 months ago

Where is the bug, though?

That it does a poor job of cleaning up lines, which looked like the purpose of the broken code.

The alternative code in the 1st comment does a better job.

But I wasn't aware that the repo was inactive. I see now. Sorry to bother you. :)