-
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
Supports local issue association with branches #3871
Supports local issue association with branches #3871
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.
Minor changes, but good after those are fixed up.
Though this should target the feature\home-remodeling
branch rather than main
src/commands/git/branch.ts
Outdated
@@ -61,6 +66,7 @@ interface CreateState { | |||
suggestNameOnly?: boolean; | |||
suggestRepoOnly?: boolean; | |||
confirmOptions?: CreateFlags[]; | |||
withIssue?: IssueShape; |
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.
withIssue?: IssueShape; | |
associateWithIssue?: IssueShape; |
src/commands/git/branch.ts
Outdated
const branch = await state.repo.git.getBranch(state.name); | ||
// TODO: These descriptors are hacked in. Use an integration function to get the right resource for the issue. | ||
const resource = issue.repository | ||
? { | ||
key: `${issue.repository.owner}/${issue.repository.repo}`, | ||
owner: issue.repository.owner, | ||
name: issue.repository.repo, | ||
} | ||
: issue.project | ||
? { key: issue.project.resourceId, id: issue.project.resourceId, name: issue.project.resourceId } | ||
: undefined; | ||
if (branch != null && resource != null) { | ||
await addAssociatedIssueToBranch(this.container, branch, { ...issue, type: 'issue' }, resource); | ||
} | ||
} |
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.
Might suggest just wrapping this completely in a function that can be reused elsewhere
src/git/models/branch.utils.ts
Outdated
return { value: undefined, paused: false }; | ||
} | ||
|
||
if (options?.cancellation?.isCancellationRequested) return { value: undefined, paused: false }; |
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.
Don't need this line because there is no await
so this can't change here
src/git/models/branch.utils.ts
Outdated
return pauseOnCancelOrTimeout( | ||
(async () => { | ||
const output = []; | ||
for (const issueDecoded of associatedIssues) { | ||
try { | ||
const issue = await getIssueFromGitConfigEntityIdentifier(container, issueDecoded); | ||
if (issue != null) { | ||
output.push(issue); | ||
} | ||
} catch (ex) { | ||
Logger.error(ex, 'getAssociatedIssuesForBranch'); | ||
} | ||
} | ||
return output; | ||
})(), | ||
options?.cancellation, | ||
options?.timeout, | ||
); | ||
} |
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 will happen serially which is fine at this point, but we probably want to make this parallel for the future
src/git/models/branch.utils.ts
Outdated
const associatedIssues: GitConfigEntityIdentifier[] = associatedIssuesEncoded | ||
? (JSON.parse(associatedIssuesEncoded) as GitConfigEntityIdentifier[]) | ||
: []; | ||
if (options?.cancellation?.isCancellationRequested || associatedIssues.some(i => i.entityId === issue.nodeId)) { |
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.
Don't need options?.cancellation?.isCancellationRequested ||
src/git/models/branch.utils.ts
Outdated
let associatedIssues: GitConfigEntityIdentifier[] = associatedIssuesEncoded | ||
? (JSON.parse(associatedIssuesEncoded) as GitConfigEntityIdentifier[]) | ||
: []; | ||
if (options?.cancellation?.isCancellationRequested) return; |
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.
Unneeded
'searchId' in entity && | ||
entity.searchId != null |
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.
Needs to be updated to check for the metadata
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 catch!
if (!isGitConfigEntityIdentifier(decoded)) { | ||
throw new Error('Invalid issue or pull request'); | ||
} |
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.
Not sure we should throw here -- could debugger;
but at runtime I think we just want to move on right?
decoded.provider === EntityIdentifierProviderType.Jira && | ||
(decoded.resourceId == null || decoded.projectId == null) | ||
) { | ||
throw new Error('Invalid Jira 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.
Same here?
0eeb7a1
to
fa4bfe2
Compare
fa4bfe2
to
c72febb
Compare
Closes #3870
In this PR:
In the next PR: