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.

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

show(::String): elide long strings (close #40724) #40736

Merged
merged 1 commit into from
May 10, 2021

Conversation

StefanKarpinski
Copy link
Member

@StefanKarpinski StefanKarpinski commented May 6, 2021

Still needs tests and I want to print the skip message in bold but printstyled doesn't nest properly, so that doesn't work. Also needs tests & news, but I thought I'd make a PR to see if this breaks any tests first.

@StefanKarpinski StefanKarpinski marked this pull request as draft May 6, 2021 15:59
@StefanKarpinski StefanKarpinski added needs news A NEWS entry is required for this change needs tests Unit tests are required for this change display and printing Aesthetics and correctness of printed representations of objects. strings "Strings!" labels May 6, 2021
@StefanKarpinski
Copy link
Member Author

Well, I have no idea why the MPFR_jll tests are failing on this PR. I've run those tests locally and they pass just fine, which suggests an interaction between the MPFR_jll tests and some other tests.

@StefanKarpinski StefanKarpinski removed needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels May 7, 2021
@StefanKarpinski StefanKarpinski marked this pull request as ready for review May 7, 2021 05:38
@StefanKarpinski
Copy link
Member Author

Well, this is fun. I cannot reproduce the MPFR_jll failure locally.

@StefanKarpinski
Copy link
Member Author

What was happening with MPFR precompilation is likely that we use repr and Meta.parse to serialize and deserialize some strings during precompilation and on CI those strings are probably longer than they are locally — long enough to cause the string elision to kick in. But we don't want that for repr ever. The correct place to do the elision is in the three-argument show(io, mime, str) method, same as how it's done for arrays. I've updated the PR to do that, so now elision only applies to display. That in turn broke a bunch of show tests — which hadn't broken before because they called methods that missed the two-arg show method. This new version is the correct one, but required fixing some tests that use long strings inside of arrays. I made them less fussy — they now only check the beginnings and endings of strings.

@fredrikekre
Copy link
Member

Shouldnt't this use get(io, :limit, false) instead of a limit kwarg?

@StefanKarpinski
Copy link
Member Author

StefanKarpinski commented May 8, 2021

The limit keyword is an integer that controls how many characters to limit the output to. This is useful for testing or if you wanted to directly call this method with a specific limit. If the limit io context were an integer or a height width tuple then it would make sense to use it for this, but it isn't. If the limit keyword arg is not passed, then the limit io context is consulted to decide whether to limit or not. So in summary:

  • If you call this show method with an explicit limit argument, it will be respected regardless of the io context.

  • If you call this show method without an explicit limit argument, as it will be called by the display system, it looks at the limit io context to decide whether to limit or not.

  • It looks at the presence or absence of a typeinfo io context in order to decide whether to print seven lines (no type info) or one line (type info). This makes sense since type info indicates that you're printing an item in a container like an array or a field in a strict, both of which are cases where you'd want one-line string printing.

@fredrikekre
Copy link
Member

Yea, behaves like I expected. My comment was based on reviewing the code before the last push.

@rfourquet
Copy link
Member

It looks at the presence or absence of a typeinfo io context in order to decide whether to print seven lines (no type info) or one line (type info). This makes sense since type info indicates that you're printing an item in a container like an array or a field in a strict, both of which are cases where you'd want one-line string printing.

Wouldn't the :compact iocontext work for that? In particular its docstring (in ?IOContext) says ":compact output should not contain line breaks."

@StefanKarpinski
Copy link
Member Author

I had that thought as well, but in my experiments, the :compact io context was not set inside of all arrays (e.g. vectors). It would be quite handy to have :limit give and actual height and width limit that you'd like printing to respect if possible. Or maybe something similar with :compact?

@StefanKarpinski
Copy link
Member Author

The main improvement I'd like here is making the ⋯ 5141 bytes ⋯ in the middle bold and maybe a different color, but it requires some version of @vtjnash's #27430 PR before that can be made to work correctly.

@StefanKarpinski
Copy link
Member Author

Since this seems to have significant support and no objections and it's ready to go, I'm merging.

@StefanKarpinski StefanKarpinski merged commit 67186f4 into master May 10, 2021
@StefanKarpinski StefanKarpinski deleted the sk/elide-long-strings branch May 10, 2021 22:18
@vtjnash
Copy link
Member

vtjnash commented May 10, 2021

It might be interesting to have a :compact flag that does something similar, but is even more substantially chopped off, for showing inside Matrixes and Tuples and such (I think Dict already does this automatically?), but this looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects. strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants