Skip to content

Conversation

@Thom1729
Copy link
Member

@Thom1729 Thom1729 commented Nov 5, 2018

Apparently the ViewStream.print function wasn't handling indentation correctly. I feel like it should have worked, but it didn't. Now it does.

This PR just implements print directly rather than using the builtin. The docstring is probably more explicit like this anyway. As a side effect, it will raise ArgumentError if the user passes file or flush arguments.

@FichteFoll
Copy link
Member

That's weird if print(file=ViewStream) doesn't work as intended. It's also significant becase we'd want to support the file protocol (it's used by various utilities in the stdlib and otherwise). Can you share some details?

The particular change seems okay, but I don't get the why.

@Thom1729
Copy link
Member Author

Thom1729 commented Nov 6, 2018

After some investigation, the following test fails:

    def test_no_indent(self):
        text = "    "

        self.stream.write(text)
        self.stream.write('\n')
        self.assertContents(text+'\n')

It looks like the insert_snippet trick isn't working. After text is inserted, the line has a four-space indent. When '\n' is inserted, another indent is inserted after the newline. This happens regardless of the auto_indent setting. However, the following test succeeds:

    def test_no_indent(self):
        text = "    "

        self.stream.write(text+'\n')
        self.assertContents(text+'\n')

This has to be a quirk of insert_snippet. It looks like to handle indentation correctly, we'll have to use insert and manage the auto_indent setting (much like we manage the read-only property). Once that's done, the change to print will be unnecessary.

@Thom1729
Copy link
Member Author

Thom1729 commented Nov 6, 2018

I've rewritten write to solve the indentation issue (hopefully) once and for all. I'm not a big fan of patching/unpatching the auto_indent setting, but it's necessary in order to preserve the originally intended behavior.

Tests have been added/updated. They will now fail if you remove the @guard_auto_indent decorator.

I think that explicit args for print still make sense, and that the docs are better with them. The implementation has been reverted to use the builtin again.

@FichteFoll
Copy link
Member

All right, this looks good. The fact that there is no way to prevent auto_indent from interfering when inserting something in the view is quite annoying, but at least we found a functioning workaround.

Left a comment regarding a test, otherwise this is fine to merge (i.e. squash-merge).

@Thom1729
Copy link
Member Author

Thom1729 commented Nov 7, 2018

In addition to splitting the read-only tests, I slightly rewrote them to use assertContents. I also improved the print tests to compare against print(file=self.stream).

@Thom1729 Thom1729 merged commit 04c41cf into master Nov 7, 2018
@Thom1729 Thom1729 deleted the print-indents branch November 7, 2018 18:14
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.

3 participants