-
Notifications
You must be signed in to change notification settings - Fork 92
Retry Fix printing of multiline values, and allow colors in showed values #345
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #345 +/- ##
==========================================
+ Coverage 93.48% 96.45% +2.97%
==========================================
Files 1 1
Lines 399 564 +165
==========================================
+ Hits 373 544 +171
+ Misses 26 20 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Probably worth adding some tests; also see the code coverage |
# I don't understand why the minus 1 is necessary here, but empircally | ||
# it is needed. |
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.
Does this "empirically it is needed" change at all with this modified calculation? Is there a test for this that verifies it is/is not needed?
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.
That's an old comment from before my PR. I did try some tests to change that value, and it causes the number of printed lines to shrink or expand each iteration if you change it.
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.
Yeah I assumed it was old, I was just wondering if it was no longer relevant since it’s now a different method for counting lines
string_value = "\e[0m" * if value isa Union{String, SubString{String}} | ||
value | ||
else | ||
repr("text/plain", value; context=IOContext(PipeBuffer(), :color => true)) |
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.
Is this repr the cause of the explicit quotes showing up?
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.
Yes. Trying to show
it like in the previous commit causes it to show up as AnnotatedString(" julia", ...)
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.
Yeah I'll throw some tests together soon, I just wanted to put the PR up for now to see if people have suggestions for how to deal with these annotated strings |
This is a continuation of #341 after it was reverted because it errored when encountering an
AnnotatedString
.Seems to be working with
AnnotatedString
.Screencast_20250405_002203.webm
The downside is that styled strings will get a
"
wrapped around them as you can see in the video, so if anyone has any better suggestions for how to deal with this generically, I'd be open to input. cc @MilesCranmer