Profile.print: color Base/Core & packages. Make paths clickable #55335
Conversation
Profile.print()Profile.print()
3abe453
to
6eef6d0
Compare
|
Orthogonal but the |
FYI I've already got code for that in #51816. |
|
I guess the truncation of paths also breaks autolinks. I'll leave the link fixing for after that PR lands. |
|
IMO the commit that adds Happy to make that PR if that would be helpful, or we can just wait. |
|
Sounds good. Doesn't need to hold this up though. @tecosaur would you mind reviewing for whether there's an easy way to implement this better with annotated strings stuff? If it requires an overhaul then maybe that could be a following PR. |
0963309
to
ee5f183
Compare
ee5f183
to
f5ca04a
Compare
Profile.print()|
File paths are now clickable in any terminal that supports it, even if they are truncated. See example up top. I opted to use IDE-specific formats over a generic URI because VSCode doesn't recognize a linenum ending on a generic URI. |
f5ca04a
to
778585b
Compare
|
Looks much more usable! I wonder if coloring the whole file path instead of just the module would make things even clearer? It has the added benefit of nicely separating the function call |
039b3f9
to
6ae0b18
Compare
|
@gdalle interesting idea..
The underline styling of links in iTerm2 makes it a little more busy than it needs to be, but I think its clearer iTerm2 does have an option to disable the underlining And it still highlights/underlines when you hover over with
|
|
I think the coloring makes the trailing |
|
With some of the colors, I find it incredibly hard to read what's really written there. For the modules, this is fine, but for the paths this could be avoided. |
|
One idea (which might be bad) would be to only colour the stacktraces that are heavier? Something like at least 5% of the samples |
|
Maybe bolding lines that have numbers on the far left would help. |
|
Yeah I think that helps
|
|
I think the
|
87a926f
to
d041e9f
Compare
|
I'm marking this for merge as I think the formatting changes have support, and the clickable links part will need some testing in the wild |
Updated ## This PR  ## master  Todo: - [ ] ~Maybe drop the `@` prefix when coloring it, given it's obviously special when colored~ If someone copy-pasted the profile into an issue this would make it confusing. - [ ] Figure out why `Profile.print(format=:flat)` is truncating before the terminal width is used up - [x] Make filepaths terminal links (even if they're truncated)
Updated ## This PR  ## master  Todo: - [ ] ~Maybe drop the `@` prefix when coloring it, given it's obviously special when colored~ If someone copy-pasted the profile into an issue this would make it confusing. - [ ] Figure out why `Profile.print(format=:flat)` is truncating before the terminal width is used up - [x] Make filepaths terminal links (even if they're truncated)
| @@ -788,9 +794,34 @@ function flat(io::IO, data::Vector{UInt64}, lidict::Union{LineInfoDict, LineInfo | |||
| return false | |||
| end | |||
|
|
|||
| # make a terminal-clickable link to the file and linenum. | |||
| # Similar to `define_default_editors` in `Base.Filesystem` but for creating URIs not commands | |||
| function editor_link(path::String, linenum::Int) | |||
There was a problem hiding this comment.
At a glance it looks like path is directly interpolated? This looks a little dodgy to me, what about a path like /tmp/?file=redherring&line=42/file.jl? Will that become say idea:https://open?file=/tmp/?file=redherring&line=42/file.jl&line=1
There was a problem hiding this comment.
That path isn't a file path? and especially not something that the profiler could return, as far as I understand...?
Though escaping the path generally seems like a good idea.
Also it'd be great to have #55454 finished to use in this function for the generic case.
There was a problem hiding this comment.
Ah I see what you're saying.. a dir named ?file=redherring&line=42. Ok
There was a problem hiding this comment.
Also it'd be great to have #55454 finished to use in this function for the generic case.
From my perspective, that PR is good to go, just awaiting further comment or merging.
In a few places across Base and the stdlib, we emit paths that we like people to be able to click on in their terminal and editor. Up to this point, we have relied on auto-filepath detection, but this does not allow for alternative link text, such as contracted paths. Doing so (via OSC 8 terminal links for example) requires filepath URI encoding. This functionality was previously part of a PR modifying stacktrace printing (#51816), but after that became held up for unrelated reasons and another PR appeared that would benefit from this utility (#55335), I've split out this functionality so it can be used before the stacktrace printing PR is resolved.
Updated
This PR
master
Todo:
Maybe drop theIf someone copy-pasted the profile into an issue this would make it confusing.@prefix when coloring it, given it's obviously special when coloredProfile.print(format=:flat)is truncating before the terminal width is used up