-
Notifications
You must be signed in to change notification settings - Fork 592
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
Edit/delete comments within the editor #600
Conversation
src/github/pullRequestManager.ts
Outdated
async editComment(pullRequest: IPullRequestModel, commentId: string, text: string): Promise<Comment> { | ||
const { octokit, remote } = await (pullRequest as PullRequestModel).githubRepository.ensure(); | ||
|
||
const ret = await octokit.pullRequests.editComment({ |
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.
We can try/catch like other methods
src/github/credentials.ts
Outdated
private _configuration: VSCodeConfiguration; | ||
private _authenticationStatusBarItems: Map<string, vscode.StatusBarItem>; | ||
|
||
constructor(configuration: any, | ||
private readonly _telemetry: ITelemetry) { | ||
this._configuration = configuration; | ||
this._octokits = new Map<string, Octokit>(); | ||
this._logins = new Map<string, string>(); |
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.
Octokit actually exposes currentUser
, it's just not shown in our typings.
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.
hmm, I'm not seeing it on the object. Do you know when it gets set/is it at the top level as octokit.currentUser
?
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.
Oh, lol. My bad, I put it on there in #554 😅
I guess I'd still suggest doing that, mostly bc it really seems like it should be there and we can probably pull it upstream.
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.
Thanks!
bb4b41f
to
78ca42e
Compare
use the new APIs to allow editing and deleting comments. I'm restricting edit/delete to only non-outdated comments that match the logged in user, but we could relax that if needed.
I'll send a separate PR for adding edit/delete actions to comments on the description page