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

Add thread support for GitLab #1140

Closed
wants to merge 1 commit into from
Closed

Conversation

shyim
Copy link
Contributor

@shyim shyim commented May 11, 2021

That would be the way I would solve the thread problem.

After your Okay @orta I can implement also Github too :)

@JoshuaBehrens
Copy link

Any activities here? I could need that one :O

this.repoMetadata.pullRequestID,
discussionId,
noteId,
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello Shym

I got a question, Why you need this ts-ignore here?

There is any cast issue with body property?

Comment on lines +201 to +202
const { id: dangerUserId } = await this.api.getUser()
const threads = await this.api.getMergeRequestThreads();
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be resolved with Promise.all

Suggested change
const { id: dangerUserId } = await this.api.getUser()
const threads = await this.api.getMergeRequestThreads();
const [{ id: dangerUserId }, threads] = await Promise.all([this.api.getUser(), this.api.getMergeRequestThreads()])

Comment on lines +103 to +108
await this.api.updateMergeRequestDiscussion(threads[0].id, threads[0].notes[0].id, newComment);
return `${this.api.mergeRequestURL}#note_${threads[0].notes[0].id}`;
} else {
const thread = await this.api.createMergeRequestDiscussion(newComment);

return `${this.api.mergeRequestURL}#note_${thread.notes[0].id}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the zero could be a const with a description. Looks like a magic number without any type of context of why the code should the zero index and not any other

@@ -67,6 +67,14 @@ export class BitBucketCloud implements Platform {
return true
}

supportsThreads() {
Copy link
Contributor

Choose a reason for hiding this comment

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

the type is infered?

@purefan
Copy link
Contributor

purefan commented Sep 29, 2022

pinging @shyim I hope you dont abandon this PR, its good work and I think the review comments are valid

@shyim
Copy link
Contributor Author

shyim commented Sep 29, 2022

As it took me too long to have it. I just built something similar of Danger for our company.

Free free to create a new PR based on this :)

@uncaught
Copy link
Contributor

@orta I suppose this is superseeded by #1331 and could be dropped?

@shyim shyim closed this Nov 16, 2022
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.

None yet

5 participants