Support {host} in hyperlinks #1901
Conversation
dandavison
force-pushed
the
support-hostname-in-hyperlinks
branch
from
be9bd21
to
9fe75db
Compare
src/cli.rs
Outdated
| /// The placeholders "{host}", "{path}", and "{line}" will be replaced by the hostname, | ||
| /// absolute file path and the line number, respectively. The default value of this option | ||
| /// creates hyperlinks using standard file URLs; your operating system should open these in the | ||
| /// application registered for that file type. However, these do not make use of the line | ||
| /// number. In order for the link to open the file at the correct line number, you could use a | ||
| /// custom URL format such as "file-line://{path}:{line}" and register an application to handle | ||
| /// the custom "file-line" URL scheme by opening the file in your editor/IDE at the indicated | ||
| /// line number. See <https://github.com/dandavison/open-in-editor> for an example. | ||
| pub hyperlinks_file_link_format: String, |
There was a problem hiding this comment.
I tried to reduce the text a bit, what about:
Placeholders "{path}" and "{line}" will be replaced by the absolute file path and the line number; "{host}" with the hostname delta is currently running on.
The default "file:" scheme creates hyperlinks using standard file URI with only the filename, which your terminal or OS should handle. You can specify any scheme, such as "file-line://{path}:{line}" and register an application to handle it. See <https://dandavison.github.io/delta/hyperlinks.html> for details.
There was a problem hiding this comment.
Perfect, thanks for doing that, I wasn't happy with it either. I've pushed a version very similar to what you wrote.
| )] | ||
| /// Format string for file hyperlinks (requires --hyperlinks). |
There was a problem hiding this comment.
While you are at it, the above (in GitHub I can't add a comment there..) should be file:///, not just //, see the mentioned RFC 8089.
There was a problem hiding this comment.
Are you saying that
default_value = "file://{path}",
is incorrect? {path} is documented as being an absolute file path, so on Unix will have a leading slash; if we change the default here to 3 slashes then wouldn't we need to make a code change to avoid 4 slashes when concatenating the path? Also we use "file-line://{path}:{line}" as an example in the text (two slashes).
I think I struggle to think in non-unix terms -- I'm guilty of thinking of "absolute path" as synonymous with "starts with slash".
There was a problem hiding this comment.
Right, the path contains the third /, then it is fine.
There was a problem hiding this comment.
Do you mind if we address this in a separate PR? I'm doubtless being stupid but it seems to me that if {path} is an absolute path, such as /etc/fstab then it's correct as-is, yielding file:///etc/fstab. I'd prefer not to make a drive-by change that's not related to the PR, without understanding the correctness implications.
There was a problem hiding this comment.
Yes, it is okay as-is, there is nothing to address :)
See debug_assert!(absolute_path.as_ref().is_absolute());
Fixes #1899.