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

Adds support for regex in autolinks #2255

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Oct 10, 2022

Description

Adds support for autolink references based on regular expression for more advanced use cases, while keeping backwards compatibily with the existing prefix-based cases.

Additionally, it refactors the Gerrit autolinks to leverage the new option:

image

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

Extras:

  • Make changes to the embedded settings browser (help welcome)
  • Properly handle escaped markdown (and probably HTML) matching

@felipecrs felipecrs marked this pull request as draft October 10, 2022 04:32
"required": [
"prefix",
"url"
"oneOf": [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This produces:

Code_McgfIrbujZ.mp4

@felipecrs
Copy link
Contributor Author

Update: we can probably use regexp-tree to parse the regex and escape it properly for markdown and html.

src/annotations/autolinks.ts Fixed Show resolved Hide resolved
@felipecrs
Copy link
Contributor Author

Ok, this is now working. The implementation may be a little rough though.

You can test with the following repo:

git clone https://review.gerrithub.io/felipecrs/gerrit-jenkins

And the last commit. It comes with a settings.json like:

{
  "gitlens.autolinks": [
    {
      "regex": "(JENKINS-[0-9]+)",
      "url": "https://issues.jenkins.io/browse/<num>"
    }
  ]
}

Demo:

Code_3ute0TidEL.mp4

@felipecrs felipecrs marked this pull request as ready for review November 21, 2022 16:04
@ajmath
Copy link

ajmath commented Dec 14, 2022

This would be awesome and open up matching on JIRA ticket ids in branch names, ex: feat_proj_123

@ahochsteger
Copy link

@eamodio, any chance to get that merged?
I just wanted to configure Gitlens once for all Jira projecs as well and was surprised that this was not working.
I mentioned you, since you originally suggested to provide a PR here: #1314 (comment) which @felipecrs already seems to have solved.

@@ -440,24 +448,42 @@ function ensureCachedRegex(
): asserts ref is RequireSome<CacheableAutolinkReference, 'messageRegex'>;
function ensureCachedRegex(ref: CacheableAutolinkReference, outputFormat: 'html' | 'markdown' | 'plaintext') {
// Regexes matches the ref prefix followed by a token (e.g. #1234)
function escapeRegexCharacters(regex: string, fn: (c: string) => string) {
return regexpTree.transform(`/${ regex }/`, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it would be too hard to achieve the same result without having to add a new dependency to vscode-gitlens 🤔

regexp-tree is really cool, nonetheless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried very hard and could not find any other way sadly.

Actually it took me a long way to reach this state even.

lol

@hongtron
Copy link

Could we clarify what changes must be made to this implementation in order to get it merged (other than resolving conflicts)? Is the added dependency a dealbreaker, or could it be acceptable as an interim solution? Would love to see this functionality hit main!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants