Skip to content

Add branchName autolinks #3644

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

Merged
merged 5 commits into from
Nov 21, 2024
Merged

Conversation

nzaytsev
Copy link
Contributor

@nzaytsev nzaytsev commented Oct 7, 2024

Description

Adds getBranchAutolinks method to Autolinks class

Adds a few tests for autolinks

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

@nzaytsev nzaytsev linked an issue Oct 7, 2024 that may be closed by this pull request
@nzaytsev nzaytsev requested a review from eamodio October 7, 2024 10:02
@nzaytsev nzaytsev force-pushed the features/3547-branch-autolinks branch from cc07324 to 9787ec2 Compare October 14, 2024 07:08
@nzaytsev nzaytsev force-pushed the features/3547-branch-autolinks branch from 9787ec2 to 333f056 Compare November 7, 2024 07:01
@axosoft-ramint axosoft-ramint self-requested a review November 20, 2024 18:51
@axosoft-ramint axosoft-ramint self-assigned this Nov 20, 2024
Copy link
Contributor

@axosoft-ramint axosoft-ramint left a comment

Choose a reason for hiding this comment

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

Gave this a test run and timed it, on a few example branches in the vscode-gitlens repo with several integrations connected (GitHub, GitLab, Jira) and it performed pretty well in my experience. Some comments and questions below from examining the code.

Also, please rebase this on the latest main when you can. There are conflicts to resolve.

Comment on lines +767 to +774
ref.branchNameRegex = new RegExp(
`(^|\\-|_|\\.|\\/)(?<prefix>${ref.prefix})(?<issueKeyNumber>${
ref.alphanumeric ? '\\w' : '\\d'
}+)(?=$|\\-|_|\\.|\\/)`,
'gi',
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my own learning/benefit, can you explain how you formulated this regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an easy regex playground service called https://regex101.com/, I always test the regexes there before I put them to the code.
Speaking about the current regex, here I expect that the link is wrapped between any star/end (split) symbols (expecting one of [-,_,.,/]). The both start/end symbols are the same except of regex symbols ^ and $. Then I expect that the issue key is a concatenation of the prefix and some non-zero count (+) of numbers (\d) or alphanumeric (\w)

@nzaytsev nzaytsev force-pushed the features/3547-branch-autolinks branch from 333f056 to a76a6a2 Compare November 21, 2024 02:47
Copy link
Contributor

@axosoft-ramint axosoft-ramint left a comment

Choose a reason for hiding this comment

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

Build errors:

image

Copy link
Contributor

@axosoft-ramint axosoft-ramint left a comment

Choose a reason for hiding this comment

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

Bug/regression: branch autolink regexes are being applied to commit messages, which is not what we want. For example, a simple number in my commit message was picked up as an issue:

image

Since you're out, I will fix it - seems like as I suggested before, we need to check referenceType in _getAutolinks like we do in _getBranchAutolinks

@axosoft-ramint axosoft-ramint self-requested a review November 21, 2024 17:14
Copy link
Contributor

@axosoft-ramint axosoft-ramint left a comment

Choose a reason for hiding this comment

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

Limited testing on this but it seems to perform well, and the regression with commit messages appears to be fixed.

@axosoft-ramint axosoft-ramint merged commit fb1f067 into main Nov 21, 2024
3 checks passed
@axosoft-ramint axosoft-ramint deleted the features/3547-branch-autolinks branch November 21, 2024 18:00
d13 pushed a commit that referenced this pull request Nov 21, 2024
* Add branchName autolinks

* update changelog

* Fix review notes

* Fixes comment, fixes commit message autolinks, makes index optional

* Fixes spacing

---------

Co-authored-by: Ramin Tadayon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Branch name "autolinks"
2 participants