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

fix: wrap pull_request_review_id in an Option #388

Merged

Conversation

0xB10C
Copy link
Contributor

@0xB10C 0xB10C commented May 31, 2023

In some instances, the GitHub API returns null for the pull_request_review_id field. Calling octocrab::pulls::PullRequestHandler::list_comments() errors with invalid type: null, expected expected ReviewId as number or string. This happens, for example, when loading the comments for PR 1772 in bitcoin/bitcoin which returns:

[
  {
    "url": "https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/1525240",
    "pull_request_review_id": null,
    "id": 1525240,
    "node_id": "MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDE1MjUyNDA=",
...

To avoid serialization problems, the field is wrapped in an Option.

In some instances, the GitHub API returns `null` for the
`pull_request_review_id` field (this seems like a bug on the
GitHub side - the API documentation lists this field as "required").
This happens, for example, here:

https://api.github.com/repos/bitcoin/bitcoin/pulls/1772/comments

To avoid serialization problems, the field is wrapped in an Option.
@0xB10C
Copy link
Contributor Author

0xB10C commented May 31, 2023

See this tester too: https://gist.github.com/0xB10C/7fb0b4c6872e0bbd65c7fe05adebf999#file-main-rs

(switching out the octocrab dependency in Cargo.toml with octocrab = { git="https://github.com/0xb10c/octocrab.git", branch="2023-05-option-pull_request_review_id" } demonstrates that this indeed fixes it. Happy to add a test-case for this too if that'd be helpful.)

@maflcko
Copy link
Contributor

maflcko commented May 31, 2023

Are you sure this is a bug? The upstream schema is:

     "pull_request_review_id": {
        "type": [
          "integer",
          "null"
        ],
        "examples": [
          42
        ]
      },

Copy link
Contributor

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

thanks, code changes lgtm, but description seems wrong?

@0xB10C
Copy link
Contributor Author

0xB10C commented May 31, 2023

Ah you're right, thanks. I had only checked if the field is "required". Updated OP.

@XAMPPRocky
Copy link
Owner

Thank you for your PR!

@XAMPPRocky XAMPPRocky merged commit 92d1ca9 into XAMPPRocky:main Jun 1, 2023
@github-actions github-actions bot mentioned this pull request Jun 1, 2023
@0xB10C 0xB10C deleted the 2023-05-option-pull_request_review_id branch June 1, 2023 10:21
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