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

[BUG] GitHubReview['state'] type does not match current GitHub review states #1443

Open
clayton-duarte opened this issue May 2, 2024 · 4 comments
Labels
bug You Can Do This This idea is well spec'd and ready for a PR

Comments

@clayton-duarte
Copy link

Describe the bug

The current GitHubReview.state type definition is "APPROVED" | "REQUEST_CHANGES" | "COMMENT" | "PENDING" | undefined.

But while trying to catch if a PR had any requested changes I found out that the state returned by GitHub is actually CHANGES_REQUESTED.

To Reproduce

Steps to reproduce the behavior:

  1. Open a new PR;
  2. Ask someone to request changes on that PR;
  3. Inside your dangerfile check the return of reviews.find((review) => review.state === 'REQUEST_CHANGES');
  4. This will not return the review you are looking for;
  5. Now try reviews.find((review) => review.state === 'CHANGES_REQUESTED');
  6. This will return yourreview;

Expected behavior

GitHubReview.state type should correctly reflect the expected strings from GitHub;

Screenshots
If applicable, add screenshots to help explain your problem.

Your Environment

software version
danger.js 11.2.8
node v18.19.0
npm 10.2.3
Operating System MacOS

Additional context
I've checked the most recent danger releases and the latest 12.1.0 release still presents this issue.

@orta
Copy link
Member

orta commented May 2, 2024

Sure, you're welcome to add it

@orta orta assigned orta and unassigned orta May 2, 2024
@orta orta added the You Can Do This This idea is well spec'd and ready for a PR label May 2, 2024
@clayton-duarte
Copy link
Author

Thanks for the quick reply @orta !

I'll be very happy to contribute, I just have a question before start working on a PR.

I was checking the codebase and found out Danger uses github's REST API. With that in mind, I was checking their documentation and I have not-so-good news. They do not have a proper enum for that field:

https://docs.github.com/en/rest/pulls/reviews#get-a-review-for-a-pull-request

So, my suggestion is to simply change GitHubReview.state to string and add some examples as a comment.

What do you suggest?

@clayton-duarte
Copy link
Author

from their response schema:

    "state": {
      "type": "string",
      "examples": [
        "CHANGES_REQUESTED"
      ]
    },

@orta
Copy link
Member

orta commented May 3, 2024

Maybe change it to be the existing literal array with | string at the end so that people can get a sense but not be complacent?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug You Can Do This This idea is well spec'd and ready for a PR
Projects
None yet
Development

No branches or pull requests

3 participants
@orta @clayton-duarte and others