Skip to content
This repository was archived by the owner on Jan 19, 2024. It is now read-only.

Conversation

@ocombe
Copy link
Contributor

@ocombe ocombe commented Apr 10, 2019

Github added a new "draft" mode for PRs, they also added new properties on events to be able to check if a PR is in draft mode or not.
This PR takes the draft mode into account in order to show the status as pending (because draft) instead of failing (because it was detected as not-mergeable, as if there was a conflict with master)

Github added a new "draft" mode for PRs, they also added new properties on events to be able to check if a PR is in draft mode or not.
This PR takes the draft mode into account in order to show the status as pending (because draft) instead of failing (because it was detected as not-mergeable, as if there was a conflict with master)
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

Hi @ocombe without any docs or tests this change is hard to review. Could you please provide these upfront prior to asking for a review?

The code looks like something that could run correctly at runtime, but the typescript implementation doesn't look right to me - I think you are working around the type system here.

I also don't understand what you mean by "(because it was detected as not-mergeable, as if there was a conflict with master)" and what role does merge_state play here.

Can you please clarify these? thanks

export interface CachedPullRequest extends Github.PullRequestsGetResponse {
pendingReviews?: number;
mergeable_state?: string;
draft?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add docs for these states? where do these states come from and why are they part of CachedPullRequest rather than PullRequestsGetResponse?

Am I missing something or are you just satisfying the compiler so that it doesn't complain about new properties due to outdated Github.PullRequestsGetResponse 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.

Yes, it's new properties that weren't available at the time.
Probot has been updated and those typings as well, we need to update the libraries to have those.

@ocombe
Copy link
Contributor Author

ocombe commented May 24, 2019

We talked about it in previous PRs but it's really hard to add tests for the bot right now, I'll need to see if we can improve that with the recent versions of probot.

The code does indeed work correctly at runtime, I tested it on my test repo and it's been in production for over a month already :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants