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 some of the missing fields to PullRequest #386

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

0xB10C
Copy link
Contributor

@0xB10C 0xB10C commented May 31, 2023

This adds some of the fields that are missing from the PullRequest model but are present in the GitHub API response. However, I haven't checked if all fields from the API are present in the model now.

The docs can be found here https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#get-a-pull-request

This adds some of the fields that are missing from the PullRequest
model but are present in the GitHub API response. However, I haven't
checked if all fields from the API are present in the model now.

The docs can be found here
https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#get-a-pull-request
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.

Are you sure those are options?

I guess it doesn't hurt too much if they are Option<>, but on upstream they seem not.

@0xB10C
Copy link
Contributor Author

0xB10C commented May 31, 2023

Based on my recent experience using the GitHub API, it's not always given that even if the field should always be set, it is always set (see e.g. #388). Though I agree that it's likely not needed here. I'll do a test-run without Options on my use-case and see if there are any nulls returned.

@0xB10C 0xB10C marked this pull request as draft May 31, 2023 18:06
@maflcko
Copy link
Contributor

maflcko commented May 31, 2023

Ok, nvm. Either way is fine by me. I think there are other places in octocrab that use Option where the upstream api field is not documented as optional.

@XAMPPRocky
Copy link
Owner

Thank you for your PR! Is there a reason you marked the PR as a draft?

@0xB10C
Copy link
Contributor Author

0xB10C commented May 31, 2023

Is there a reason you marked the PR as a draft?

Wanted to verify if the Option around the fields is needed before this is merged.

Turns out, it's needed. The API does not return e.g. additions when listing pull requests with https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests but does return additions when getting a single pull request with https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#get-a-pull-request.

@0xB10C 0xB10C marked this pull request as ready for review May 31, 2023 19:18
@XAMPPRocky
Copy link
Owner

Thank you for your PR!

@XAMPPRocky XAMPPRocky merged commit 34bb086 into XAMPPRocky:main Jun 1, 2023
@github-actions github-actions bot mentioned this pull request Jun 1, 2023
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