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

Edit/delete comments within the editor #600

Merged
merged 2 commits into from
Oct 26, 2018
Merged

Conversation

RMacfarlane
Copy link
Contributor

comment_edit

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

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({
Copy link
Member

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

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>();
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

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.

3 participants