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.

Sign up for GitHub

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

Jump to bottom

Added echo bool to capture. #2338

Open
willmcgugan opened this issue Jun 14, 2022 · 6 comments · May be fixed by #2347
Open

Added echo bool to capture. #2338

willmcgugan opened this issue Jun 14, 2022 · 6 comments · May be fixed by #2347
Assignees
@darrenburns
Labels
Needs triage

Comments

@willmcgugan
Copy link
Collaborator

The capture method should have an echo parameter. When echo=True (the default) text should be captured and written to the terminal. When echo=False the text should be captured but not written to the terminal.

See #2172 for context.
@darrenburns darrenburns self-assigned this Jun 15, 2022
@darrenburns
Copy link
Member

I think #2172 is unrelated to what actually gets written to the terminal and is instead about record=True not recording things that occur inside a Capture context. Doesn't impact developing this feature, just thought I'd point it out.

@darrenburns darrenburns linked a pull request Jun 16, 2022 that will close this issue
Add echo param to Console.capture #2347
Open
9 tasks
@SertugF
Copy link

SertugF commented Dec 6, 2022

Hello, i have been working on this. If echo is true by default it breaks the test below. Since new capture method prints all captured strings 'out' changes.

def test_capture_and_record(capsys):
    recorder = Console(record=True)
    recorder.print("ABC")

    with recorder.capture() as capture:
        recorder.print("Hello")

    assert capture.get() == "Hello\n"

    recorded_text = recorder.export_text()
    out, err = capsys.readouterr()

    assert recorded_text == "ABC\nHello\n"
    assert capture.get() == "Hello\n"
    assert out == "ABC\n"

terminal output :
image

In order to pass previous tests should i just change default 'echo' value to 'false' ? Or does this test needs to be replaced ?

@ssbarnea
Copy link
Contributor

ssbarnea commented Dec 6, 2022

IMHO, inside the test we should be explicit about echo argument, regardless what is the default value, especially as this is expected to change the final outcome.

@SertugF
Copy link

SertugF commented Dec 9, 2022

Okay, below is what you recommend then ?

def test_capture_and_record(capsys):
    recorder = Console(record=True)
    recorder.print("ABC")

    with recorder.capture(echo=False) as capture:
        recorder.print("Hello")

    assert capture.get() == "Hello\n"

    recorded_text = recorder.export_text()
    out, err = capsys.readouterr()

    assert recorded_text == "ABC\nHello\n"
    assert capture.get() == "Hello\n"
    assert out == "ABC\n"

While trying solve the problem, i have seen that even tho my code works for single recorder.print() call, it actually breaks when there is multiple of them. Since there is no test for that. It passes. So i have made test below in order to future-proof it. Can somebody give some feedback about it.

def test_capture_and_record_multiple(capsys):
    recorder = Console(record=True)

    with recorder.capture(echo=False) as capture:
        recorder.print("Hello")
        recorder.print("World")

    assert capture.get() == "Hello\nWorld\n"

    recorded_text_echo_false = recorder.export_text()
    out, err = capsys.readouterr()

    assert recorded_text_echo_false == "Hello\nWorld\n"
    assert out == ""

    with recorder.capture(echo=True) as capture:
        recorder.print("Foo")
        recorder.print("Bar")

    assert capture.get() == "Foo\nBar\n"

    recorded_text_echo_true = recorder.export_text()
    out, err = capsys.readouterr()

    assert recorded_text_echo_true == "Hello\nWorld\nFoo\nBar\n"
    assert out == "Foo\nBar\n"

@ssbarnea
Copy link
Contributor

ssbarnea commented Dec 9, 2022

THe only hint I know about capturing standard streams is that your do not want to join them, especially for testing. If you ever do this, the order can be anything as each of them does have its own buffering. Even if there are some controls for buffering they often do not work as intended, being known that on several cases you cannot really disable the buffering.

@SertugF
Copy link

SertugF commented Dec 9, 2022 •
edited

I see, calling readouterr twice in same test is unreliable. Below is new version:

def test_capture_and_record_multiple(capsys):
    recorder = Console(record=True)

    with recorder.capture(echo=False) as capture:
        recorder.print("Hello")
        recorder.print("World")

    assert capture.get() == "Hello\nWorld\n"

    with recorder.capture(echo=True) as capture:
        recorder.print("Foo")
        recorder.print("Bar")

    assert capture.get() == "Foo\nBar\n"

    out, err = capsys.readouterr()

    assert out == "Foo\nBar\n"

Another thing which ı would like to ask is about recorder.export_text() function. It is defined as follows :

def export_text(self, *, clear: bool = True, styles: bool = False) -> str:
        """Generate text from console contents (requires record=True argument in constructor).

        Args:
            clear (bool, optional): Clear record buffer after exporting. Defaults to ``True``.
            styles (bool, optional): If ``True``, ansi escape codes will be included. ``False`` for plain text.
                Defaults to ``False``.

        Returns:
            str: String containing console contents.

        """
        assert (
            self.record
        ), "To export console contents set record=True in the constructor or instance"

        with self._record_buffer_lock:
            if styles:
                text = "".join(
                    (style.render(text) if style else text)
                    for text, style, _ in self._record_buffer
                )
            else:
                text = "".join(
                    segment.text
                    for segment in self._record_buffer
                    if not segment.control
                )
            if clear:
                del self._record_buffer[:]
        return text

Above function creates unwanted behavior while working with multiple captures. Example :


with recorder.capture(echo=False) as capture:
    recorder.print("Hello")
    recorder.print("World")

# when there is more than one record it doesnt work. captures first record twice
recorded_text = recorder.export_text()

str = capture.get()

if recorded_text == "Hello\nWorld\n":  #  test  returns : 'Hello\nHello\nWorld\n'
    print("Test1: Success")
else:
    print("Test1: Failed")

Is this expected behavior ? I thought my code was the cause of it but even in master without echo argument it creates "'Hello\nHello\nWorld\n'" output. The first written test for capture called "test_capture_and_record" and it is using it. So i thought maybe i should also use it but in its current form i dont think it works correctly. Should i try to fix it or just skip it ? I do think main problem is below code, it seems to be doing some extra loops inside for.

 text = "".join(
                    segment.text
                    for segment in self._record_buffer
                    if not segment.control

@SertugF SertugF mentioned this issue Dec 9, 2022
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@darrenburns darrenburns

Labels
Needs triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add echo param to Console.capture Textualize/rich
4 participants
@ssbarnea @willmcgugan @darrenburns @SertugF