Add configurable hyperlinks #2483
Conversation
|
Works well for me. One minor thing I noticed, is that the last colon ist embedded in the link label. Visually it would be more pleasant to exclude that, but opinions on that might differ. Thanks for implementing this. |
|
And one more minor thing. The column seems to be off by one. I would assume it to be zero-based. |
|
Thanks for your feedback! 🙂
Yes, I thought about it, and I think you're right - it would probably look better. But I didn't want to change the existing code too much: currently there is one function per field (file/line/column/byte offset), which writes the field value followed by its separator (it may be different for the file). I'd have to break that design in order to end the hyperlink right before the last separator. I'll see if I can come up with something clean though.
It works fine for me, at least in VSCode. Maybe your editor uses zero-based columns? In that case, I may have to add a |
If it would require major modifications, I think it can just stay like that. It's really minor for me.
I'm using emacs. I'm not sure if there is support for custom URIs like with vscode. I use my own scripts so it's not that important for me. I can just substract one from each passed column. I guess I was wrong here. I just found it odd that something is 1-based, but lines are 1-based too. So I would wait for other people to report issues with that before you implement a switch for that. 🙂 |
ltrzesniewski
force-pushed
the
hyperlinks-cfg
branch
2 times, most recently
from
13d08cd
to
15140b4
Compare
Hyperlinks in
In order to open results to a specific file and line when clicking on hyperlinks, I created a custom scheme ( It allows to open files to a specific line (or column, but I've not added column support in |
|
@misaki-web Thanks, I added the @fabian-thomas I made the hyperlink end before the last field separator, and it looks better indeed:
|
|
@BurntSushi since I know you're careful about the dependency tree of ripgrep, and this PR currently adds a dependency to (note that this PR also adds a dependency to |
|
I haven't reviewed this PR yet. There is a lot of code here. It may be warranted, but I really just haven't sat down to explore the design space myself yet.
|
|
No worries, I understand it's a large change and I don't want to rush you. 🙂 I'll make the switch to |
|
Thanks for making (iTerm2 can be set, in the fragment case (but not for all links, regardless of fragment status), to use Semantic History rules for hyperlinks, which lets you run a coprocess when they're clicked on instead of passing to the OS, which lets me stack several more hacks to get the link to open in a vim pane in the current tmux window.) |
|
I'm glad you find it useful! 😄 May I ask you a little question, since you seem to be a proficient macOS user? Are you aware of any popular macOS apps which provide custom URL handlers that users may want to use to open ripgrep links? I could add them to the aliases list in this PR, as it's currently a little short. |
|
I think there are probably quite a few, which is why the configurability is good. Here are the editors iTerm2 has for opening files with: 
I'm not sure how many of them have their own path handlers. I know a few people who use MacVim, which has one. I think Atom has fallen out of favor, while Sublime is reasonably popular. If you're adding built-in support for vscode, might as well add vscodium as well. If you're really trying to pack features in, you could add a git remote URL scheme that hyperlinks to |
|
Great, thanks for this list! I've added a few aliases. I'm not really trying to pack features in, but I thought having a built-in list of common URL patterns would be nice to have. 🙂 |
|
It's bad juju to merge master back into a feature branch. There's 35 commits here. I'm presuming this is an artifact of development. If so, please rebase this onto master and squash it down to one commit. If that is too much work, then I would say skip it because I haven't reviewed this PR yet and I dunno if I will accept it. A 1K line diff is a red flag to me. |
|
Well they are writing a new feature in and tried to do it in a very dynamic/generic way rather than just hardcoding a single hyperlink format. I am also guessing they expected the squash+merge but it shouldn't be an issue to rebase onto master with a single commit. POC at: mitchcapper/ripgrep@master...pr_rebased As for the number of lines most is the new hyperlink class (almost 700 of the 1000) with the largest number of changes in the standard printer where 130 of the 200 new lines are all together for the new structured writer. It is a good bit of code but at least not massive changes to a lot of existing code. Full stats for the PR are: Hopefully it is considered, it is a nice addition |
|
Sorry, I thought you'd rather have a PR without merge conflicts. Yes, I totally expected to squash this into a single commit, and will do so right now since you prefer it that way. But it depends on BurntSushi/termcolor#65, which means it still has a few changes that can't be merged as-is (in the CI and Cargo.toml). It's ready for review though. I understand it's a large change, but it adds a brand new feature, which I tried to keep isolated. As @mitchcapper points out, most of the new code is in a separate file. I did my best to write good code, with comments and tests, and that unfortunately doesn't make it short. |
ltrzesniewski
force-pushed
the
hyperlinks-cfg
branch
from
bd8f0ec
to
ce49a7d
Compare
|
Thanks. Yes, no conflicts is good. But not with merge commits. :-) Check out the commit history of ripgrep. There are no merge commits anywhere. (Except for the beginning IIRC.) I appreciate the work done here. I don't even know whether I'm going to support hyperlinks at all yet. I think it's likely, but it's something I have to sit down and think through myself. I understand y'all did a lot of that work already, but once I merge something like this, it then becomes my responsibility. So it has to be something I'm happy to continue to maintain indefinitely. Anywho, I'm going through the process of clearing out the backlog so that I can put out a new release, and I plan to review this soon. But it could take some time. The partial point of my comments here are to set expectations. I generally prefer folks to get an OK from me before spending so much time on something like this for that reason. I hate to see folks spend a bunch of time on work that doesn't wind up getting merged. I realize this is difficult to do because I have bursts of activity at unpredictable points. My schedule and workflow for my free time projects unfortunately makes a lot of my projects less friendly to contributions than I would like. |
|
No worries, I understand the expectations very well, as I have similar ones for my own projects. You seemed to be open to this feature, and I really wanted to have it in a tool I use every day. Besides, developing this helped me tremendously with learning Rust, so I consider this a very positive experience even if it doesn't end up being merged. Once you think it through, I'll be happy to implement any changes you'll request until you're satisfied. That'll be another opportunity to play with the language. 🙂 |
|
Hi @ltrzesniewski, re #86 (comment) the JetBrains URL protocols I'm aware of are documented at https://dandavison.github.io/delta/grep.html (I don't remember where I got them from but I remember testing it with PyCharm at least). |
|
I've done a very quick review, and I have to say, this is an extremely high quality PR. It looks like you put a ton of effort into this. I like the idea of configuring hyperlink format as you've done since it doesn't seem like we can get away with just one format. I need to do a deeper review of how everything fits together, but I'd say I'm pretty likely to accept this PR or something close to it. You don't need to do anything from this point I think, I'll take it over from here. I do have a high level question though: are these hyperlinks enabled by default? If so, how necessary do you think that is? My problem is that, AIUI, emitting hyperlinks requires path canonicalization, which I perceive to be a somewhat heavyweight operation. I'm not sure I want that to happen by default (even if it's just when writing to a tty). |
|
Thanks for your kind words, I'm glad you like it! 🙂
Currently they're enabled by default when color is enabled and termcolor supports them (which means we're not using the old Windows console host).
I think that's the most user-friendly solution. I don't expect most users to configure
This is done once per matching file, and then cached for subsequent matches in the same file: I added a I admit I didn't run any benchmark, but I wouldn't expect this to be an issue. |
|
The
There's also the problem that So I will probably:
|
|
Yeah, even though I am on Linux, I don't really have links-in-terminals setup. I don't expect to be a user of this feature in general. The only "GUI" I use is a web browser, and very occasionally So I'm probably just going to have to stick with canonicalize for now, since that seems to give the best chance of the link actually working. |
|
Just to be clear, Windows is the easy case in terms of |
|
Right. The issue is knowing (on Unix, where handling |
|
It's not quite clear to me where this theoretical |
|
Just what the user types. For example, |
|
@ltrzesniewski In your PR, I found this comment about constructing the hyperlink on Unix: // On Unix, this function returns the absolute file path without the
// leading slash, as it makes for more natural hyperlink format, for
// instance:
//
// file://{host}/{file} instead of file://{host}{file}
// vscode://file/{file} instead of vscode://file{file}
//
// It also allows for the format to be multi-platform.I am trying to decide whether it makes sense to strip the leading I guess from my view, I would rather the variables be easier and more intuitive to understand as individual pieces than to make the format as a whole a little more natural to look at. When someone is trying to write a new hyperlink format, it will require reasoning about each individual part, and I'm also thinking of renaming |
|
If we leave the leading slash on Unix, I think the implication is that I would add a leading slash on Windows. Which I guess also feels a little unintuitive to me from the Windows perspective. There, you'd expect an absolute local path to start with a drive letter I think. But maybe this is less bad than removing the leading slash on Unix? Another way to think about it is, as I said above, that |
|
Well TBH I'd prefer removing the leading slash, because it feels more natural and intuitive that way IMO. You'd have to write
On the other hand, if you don't prefix the path with a slash on Windows, the patterns would become platform-specific, and you'd have to write them twice in But more importantly, you know the path will be canonicalized and are reasoning with that in mind, but I wouldn't expect the users to think about this detail. They may expect That's why I think a pattern such as
As you prefer. Both seem fine to me. |
|
Yes, I would add a leading slash to Windows paths. I've tried this out and I like it best among all choices that I can think of. I don't see any one choice that leads to good and intuitive behavior in all cases. I don't think there are any cases where you need to write
The docs for the format will need to explicitly mention that My reasoning for sticking closer to the RFC is that, in my experience with these sorts of things, the RFC winds up serving as a lowest common denominator. And if we introduce concepts that diverge from the RFC, then it's plausible they can be combined in ways we didn't anticipate (perhaps with future features) that lead to a worse experience than if we just stuck with the RFC. This is a very hand-wavy argument and I could absolutely be wrong, but this is just what my experience says in these sorts of scenarios. Finally, ideally, most users won't need to actually write their own hyperlink format and can instead select from one of the existing pre-defined formats. I expect that list to grow over time too. |
|
The next change I'm looking to make involves the syntax of the format. I like the interpolation syntax, And we'll also want to make it possible to escape special characters. Just like |
Maybe we could improve the error message instead, for instance by providing a list of known aliases when there's no But since those aliases would mainly be used in a config file, the Also
Yes, I also thought about that, but in the end I didn't think it was necessary since I didn't expect I suppose |
|
My frame of reference here are these two points:
I'll continue to noodle on the syntax. |
|
Actually, a simple solution could be:
|
|
I wonder if it makes sense to include So I guess, under what conditions does this ..... Now that I've typed all of that out, I realize the I could find very little documentation or acknowledgment of how WSL paths fit into file URIs. Maybe I'm not looking in the right place. |
Yes, WSL2 runs a real Linux kernel under Windows, and it can execute standard Linux binaries. We're in
Sure, you can just skip it. I added this as a little convenience feature because I thought it would be nice to have and easy to implement. I suppose people who use ripgrep under WSL will know how to customize their hyperlink patterns. Sorry for the additional trouble. 😅 |
|
No no, not trouble at all. It's great that you put that in there, because it would have been an unknown unknown otherwise. I think what I'll do is provide a |
|
If you're curious about WSL paths, here's an example of what the Windows Explorer shows:
It shows Since you can get access to the Linux filesystem in the same way as a remote CIFS share on the network,
That would be nice to have, since you can't expand environment variables in ripgrep config files, so you'd have to hardcode the distro name without it.
Yes, you'd have to replace That's why I think having a And I agree that having just a plain
Yeah, I don't know either. 😕 Both options would work, but I initially put it into I suppose having a WSL-specific placeholder would be the better option, as if you prefix it to |
|
Gotya. Yeah, there are a lot of options here. We could also add a |
This essentially takes the work done in #2483 and does a bit of a facelift. A brief summary: * We reduce the hyperlink API we expose to just the format, a configuration and an environment. * We move buffer management into a hyperlink-specific interpolator. * We expand the documentation on --hyperlink-format. * We rewrite the hyperlink format parser to be a simple state machine with support for escaping '{{' and '}}'. * We remove the 'gethostname' dependency and instead insist on the caller to provide the hostname. (So grep-printer doesn't get it itself, but the application will.) Similarly for the WSL prefix. * Probably some other things. Overall, the general structure of #2483 was kept. The biggest change is probably requiring the caller to pass in things like a hostname instead of having the crate do it. I did this for a couple reasons: 1. I feel uncomfortable with code deep inside the printing logic reaching out into the environment to assume responsibility for retrieving the hostname. This feels more like an application-level responsibility. Arguably, path canonicalization falls into this same bucket, but it is more difficult to rip that out. (And we can do it in the future in a backwards compatible fashion I think.) 2. I wanted to permit end users to tell ripgrep about their system's hostname in their own way, e.g., by running a custom executable. I want this because I know at least for my own use cases, I sometimes log into systems using an SSH hostname that is distinct from the system's actual hostname (usually because the system is shared in some way or changing its hostname is not allowed/practical). I think that's about it.
This commit represents the initial work to get hyperlinks working and was submitted as part of PR #2483. Subsequent commits largely retain the functionality and structure of the hyperlink support added here, but rejigger some things around.
This essentially takes the work done in #2483 and does a bit of a facelift. A brief summary: * We reduce the hyperlink API we expose to just the format, a configuration and an environment. * We move buffer management into a hyperlink-specific interpolator. * We expand the documentation on --hyperlink-format. * We rewrite the hyperlink format parser to be a simple state machine with support for escaping '{{' and '}}'. * We remove the 'gethostname' dependency and instead insist on the caller to provide the hostname. (So grep-printer doesn't get it itself, but the application will.) Similarly for the WSL prefix. * Probably some other things. Overall, the general structure of #2483 was kept. The biggest change is probably requiring the caller to pass in things like a hostname instead of having the crate do it. I did this for a couple reasons: 1. I feel uncomfortable with code deep inside the printing logic reaching out into the environment to assume responsibility for retrieving the hostname. This feels more like an application-level responsibility. Arguably, path canonicalization falls into this same bucket, but it is more difficult to rip that out. (And we can do it in the future in a backwards compatible fashion I think.) 2. I wanted to permit end users to tell ripgrep about their system's hostname in their own way, e.g., by running a custom executable. I want this because I know at least for my own use cases, I sometimes log into systems using an SSH hostname that is distinct from the system's actual hostname (usually because the system is shared in some way or changing its hostname is not allowed/practical). I think that's about it. Closes #665, Closes #2483
This commit represents the initial work to get hyperlinks working and was submitted as part of PR #2483. Subsequent commits largely retain the functionality and structure of the hyperlink support added here, but rejigger some things around.
|
I implemented a gui wrapper to it. you can download it at https://github.com/garawaa/ripgrep-gui with result files:
|
This PR adds hyperlinks to search results in terminals which support them:
Compared to the previous PR (#2322), it adds a
--hyperlink-format PATTERNcommand line option which lets you configure the output.PATTERNcan be:{file}for the absolute file name. This one is required./on Unix (as this makes the patterns more intuitive and multi-platform)\replaced by/on Windows{line}for the line number of the result.{column}for the column number of the result.{line}is required if this is used.{host}for the hostname. This becomeswsl$/{distro}on WSL.vscode. Only a few are currently defined but hopefully more could be added over time, on the same principle as file types (default_types.rs). A few special aliases are included:filefor the defaultfile://schemenoneto disable hyperlinks (it aliases to the empty string)The default patterns are the ones which are the most widely supported:
file://{host}/{file}file:///{file}file://wsl$/{distro}/{file}This makes ripgrep only output hyperlinks on the file headings by default, like in the screenshot above.
If a patten includes
{line}however, each line prelude becomes a link. Here's an example with--hyperlink-format vscode:The idea is for a user to add
--hyperlink-formatto their configuration file to enable more features.This PR depends on BurntSushi/termcolor#65 to write hyperlink control codes to the terminal. I had to make changes to the CI in order to build ripgrep without that PR having shipped. I'll revert them once the updated termcolor crate is published. Of course, if any change is requested there, I'll update this PR accordingly.
I tried my best to write high-quality code, I hope it's good enough, but I'd appreciate any feedback on how I could have done things better, since Rust isn't my day-to-day language. 🙂
Please ignore the long commit history, I'll either clean it up or just squash it once the changes are approved.
Closes #665
Supersedes #2322