-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add echo
param to Console.capture
#2347
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2347 +/- ##
==========================================
- Coverage 98.71% 98.66% -0.05%
==========================================
Files 73 72 -1
Lines 7704 7721 +17
==========================================
+ Hits 7605 7618 +13
- Misses 99 103 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can update the docs to describe the new echo
param please.
rich/console.py
Outdated
@@ -723,6 +727,7 @@ def __init__( | |||
self._thread_locals = ConsoleThreadLocals( | |||
theme_stack=ThemeStack(themes.DEFAULT if theme is None else theme) | |||
) | |||
self._buffer_indices_to_render: Set[int] = {self._thread_locals.buffer_index} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't quite follow what the intent of this is. Handling nested captures? Can you explain to me on Tuesday please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously we only rendered content if the buffer index was 0. This was a hard-coded check. Now we also want to do this if we're inside a capture context (the buffer index is not 0) and where the echo
arg is True
, so we record the indices where that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few requests.
@@ -368,6 +368,19 @@ def test_capture_and_record(capsys): | |||
assert out == "ABC\n" | |||
|
|||
|
|||
def test_capture_echo_outputs_captured_content_to_terminal(capsys): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test with more nesting. e..g. a capture inside a capture. Or a simple with console:
block within a capture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried this test and it passes on master
(it shouldn't), so I'm not sure nested captures work at all:
def test_capture_echo_nested_capture():
console = Console()
console.print(1)
with console.capture() as capture1:
console.print(2)
with console.capture() as capture2:
console.print(3)
console.print(4)
assert capture1.get() == ""
assert capture2.get() == ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering what the expected behaviour should be.
The outer capture prevents content from getting to the terminal. So the inner capture has nothing to capture? So maybe this is the expected behaviour.
Could you also test this:
with console.capture():
with console:
console.print("foo")
The with console should make the block atomic, i.e. it saves up all content until it exists the block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The outer capture prevents content from getting to the terminal. So the inner capture has nothing to capture? So maybe this is the expected behaviour.
I'd still expect the 2
to be captured inside capture1
.
This test passes:
def test_capture_echo_nested_capture_console():
console = Console()
with console.capture() as capture1:
console.print(1)
with console:
console.print(2)
assert capture1.get() == "1\n2\n"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scrap this, I think things are working as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now that I've refreshed my memory with this stuff 😄 ...
This is the behaviour on master
, which looks wrong:
def test_capture_echo_nested_capture():
console = Console()
console.print(1)
with console.capture() as capture1:
console.print(2)
with console.capture() as capture2:
console.print(3)
console.print(4)
assert capture1.get() == ""
assert capture2.get() == "2\n3\n"
My expectation would be that capture1
captures 2\n3\n
and capture2
captures 3\n
.
Just wanted to see if |
@nathanrpage97 I agree, I don't think |
Yea, I guess it may be a bit esoteric for the general users of the library |
@darrenburns Any chance to revive this? The changes introduced by 12.5.1 broke capture support in enrich, pycontribs/enrich#40 and temporary I had to pin-down rich as I would need the echo/tee functionality. Thanks. |
@willmcgugan That PR happens to be on xmas wishlist for quite some time. I wonder if Santa is feeling generous these days ;) |
@darrenburns any chance to update it? It would be quite cool to see it in. |
Type of changes
Checklist
Description
Adds
echo
parameter to thecapture
method ofConsole
. WhenTrue
, captured output will also be written to the output file.