PyCQA / redbaron

Bottom-up approach to refactoring in python
http://redbaron.pycqa.org/
694 stars 74 forks source link

Make the handling of inserted newlines more consistent #144

Open chadrik opened 7 years ago

chadrik commented 7 years ago

This removes a bunch of special-case addition of newlines and attempts to preserve exactly the number provided by the user when inserting or setting. It also fixes #48, where inserting a line after a 'def' could produce additional newlines after the 'def' block.

The erroneous newline triggered by insertion (issue #48) is due to this:

        if not expected_list or not isinstance(expected_list[-1], (CodeBlockNode, redbaron.nodes.EndlNode)):
            log(
                ">> List is empty or last node is not a CodeBlockNode or EndlNode, append a separator to it and set identation to it")
            expected_list.append(generate_separator())
            expected_list[-1].indent = last_indentation
            log("-- current result: %s", ["".join(map(lambda x: x.dumps(), expected_list))])

The sensible solution seemed to be to make IfelseblockNode inherit from CodeBlockNode (it's a code block after all), which removed the stray newline, but that made the following test fail:

def test_line_proxy_correctly_indent_code_block():
    red = RedBaron("while True:\n    pass\n")
    red[0].extend(["if a:\n    pass\n\n"])
    assert red.dumps() == "while True:\n    pass\n    if a:\n        pass\n\n"

The new result lost the final newline. This led me into CodeBlockNode.parse_code_block, where I discovered a lot of questionable (to an outsider) removal and addition of newlines.

It starts with:

            clean_string = "    " + "\n    ".join(clean_string.split("\n"))
            clean_string = clean_string.rstrip()

Note that the call to strip removes whitespace and newlines.

Then there was:

        fst = baron.parse("def a():\n%s\n" % clean_string)[0]["value"]

The %s\n adds a newline.

Then there was a bunch of code trying to guess how many newlines to add back, without taking into consideration how many there were to begin with. I ditched all of that. This means that this PR changes the expected results of the setter tests, but it does so in a way that is truer to the user's request.

e.g.

    c.value = "return 42\n"

Now inserts a single newline as requested, instead of 3.

There is a very good chance that I've misunderstood something, since I've only just started using redbaron. Please let me know if that is the case.

webknjaz commented 4 years ago

@Psycojoker ^