-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improves branch autolinks experiments #3898
base: main
Are you sure you want to change the base?
Conversation
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.
I don't quite follow the logic of the added code (specifically, the priority calculation) - some comments throughout might help. @eamodio might have more insight into it.
It didn't seem to help with this case I encountered in my repo. Not a blocker for merge but would be nice if it can accommodate cases like this (the first issue is correct, the second is not):
src/autolinks/autolinks.utils.ts
Outdated
console.log( | ||
JSON.stringify(match.value), | ||
match.value.groups.numberChunk, | ||
match.value.groups.numberChunkBeginning, | ||
match.value.input, | ||
match.value.groups.issueKeyNumber, | ||
); |
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.
This console log should be removed. Likely left over from when you were testing it locally.
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.
removed
625e1ab
to
8e05baa
Compare
66a9fd1
to
e082063
Compare
@axosoft-ramint should this one make it into 16.2 or push to the next release? |
@nzaytsev To help me review this PR, please explain this a bit:
Some specific examples, including what would happen before your PR and after your PR, and an argument for why you believe this is the case:
...would help me a lot. Thanks! |
@axosoft-ramint - all these theses are my assumptions based on our discussions and on my experience or feeling. All of these examples was created to "tune" the manual parsing algorithm. I realize that these theses are not always true, but we can not tune this algorithm to be working always for all users and all use cases. There are many contradictory cases and I still don't know how to handle Speaking more contcrete
Dec 20th, 2024 at 9:43 AM, when we was discussing to numbers in the branch name
But I didn't agree with that 1 is more likely the issue number than 16 and I proposed number chunks. So, now 16-1 is a number chunk where the meaningful part is the first. Sometimes devs can call their branches like below:
Here I applied the breadcrumbs logic. Maybe devs work on an epic with the key X and have a scope of issues inside the epic:
|
Description
Added chunk matching logic for non-prefixed numbers:
considered the distance from the edges of the chunk as a priority sign
the chunk that is more close to the end is more likely actual issue number
Checklist
Fixes $XXX -
orCloses #XXX -
prefix to auto-close the issue that your PR addresses