-
Notifications
You must be signed in to change notification settings - Fork 393
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
Allow multiple hyperlinks per line #1909
Conversation
src/features/hyperlinks.rs
Outdated
let formatted_commit = | ||
format_osc8_hyperlink(&commit_link_format.replace("{commit}", commit), commit); |
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.
What about filtering out pure numbers here again, as mentioned in the issue?
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, why not. The posterior probability of such a string being a number is going to be considerably higher than that of it being a commit. Multiplied by the probability that someone happened to care about the link to their improbably-digit-rich commit...
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.
Done.
src/features/hyperlinks.rs
Outdated
commit = commit, | ||
osc = "\x1b]", | ||
st = "\x1b\\" | ||
); |
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 there a reason that we can't use format_osc8_hyperlink
here also? And beyond that, The two branches of this function are quite substantial and look very similar -- did you decide it wasn't attractive to unite them?
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.
Right, just didn't want think about how to nicely factor that out :) - What about this HyperlinkCommits
helper now?
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.
Nice, the helper LGTM.
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.
Great! Made a couple of comments but feel free to merge when you're happy with it.
src/features/hyperlinks.rs
Outdated
format!( | ||
"{osc}8;;{url}{st}{commit}{osc}8;;{st}", | ||
url = repo.format_commit_url(commit), | ||
commit = commit, | ||
osc = "\x1b]", | ||
st = "\x1b\\" |
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.
Can be replaced with format_osc8_hyperlink()
, right?
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 I believe so. I'm not sure why the previous code wasn't already using that. Hopefully just a historical accident rather than a Chesterton's fence.
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.
Done (tests agree)!
Previously only the last commit was linked. Do not link numbers (technically also commits), and stop after finding 12 commits on a line.
Previously only the last commit was linked
Do not link numbers (technically also commits), and stop after
finding 12 commits on a line.
Fixes #1907
I'd want to enhance all the current assertions with pretty_assertions, that is no work compared to using insta.