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

Support pulls/{number}/commits #656

Merged
merged 1 commit into from
Sep 1, 2024
Merged

Conversation

SamWilsn
Copy link
Contributor

No description provided.

@XAMPPRocky
Copy link
Owner

Thank you for your PR! Can you rebase it on top on main?

@SamWilsn
Copy link
Contributor Author

Done!

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.

lgtm

@SamWilsn
Copy link
Contributor Author

@XAMPPRocky So it seems like GitHub's schema isn't entirely correct for the Author type, or maybe I'm doing something wrong. For at least one pull request, I'm getting back JSON that's deserializing to Author that doesn't have login or id. Have you encountered this before?

@XAMPPRocky
Copy link
Owner

@SamWilsn Yes, GitHub's schema is descriptive at best. It doesn't match the reality of the API and what the API will actually return.

@SamWilsn
Copy link
Contributor Author

Should I change the Author type to have optional fields, or make a new type specific to this endpoint?

@XAMPPRocky
Copy link
Owner

Make it have optional fields, there might other endpoints where this also happens.

@SamWilsn
Copy link
Contributor Author

It's, uh... mostly Option now 😅

src/models.rs Outdated
pub r#type: String,
pub site_admin: bool,
#[serde(skip_serializing_if = "Option::is_none")]
pub login: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure this is correct?

author inside a commit is optional, but the fields inside author inside commit are not.

If you disagree, please link to the GitHub API schema that shows that the fields in author are optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a GitUser, not an Author, see:

/// Metaproperties for Git author/committer information.
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct GitUser {
    pub date: Option<String>,
    pub email: Option<String>,
    pub name: Option<String>,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't provide a schema, but here's an API response:

You can get the schema from https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-commits-on-a-pull-request . See tab "Response schema".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. On this commit we have:

{
  // ...
  "comments_url": "https://api.github.com/repos/ethereum/ERCs/commits/fb8277911c110491bf3610b5e0298c4f4cb70a3f/comments",
  "author": {

  },
  "committer": {

  },
  "parents": [ /* ... */ ],
  // ...
}

Note the empty (but non-null) committer and author items.


On another commit we have:

{
  // ...
  "comments_url": "https://api.github.com/repos/ethereum/ERCs/commits/ce626a0f5d26545ececf4ca6ea4bf64c16b6b233/comments",
  "author": {
    "login": "tokernomics",
    "id": 88177870,
    "node_id": "MDQ6VXNlcjg4MTc3ODcw",
    "avatar_url": "https://avatars.githubusercontent.com/u/88177870?v=4",
    "gravatar_id": "",
    "url": "https://api.github.com/users/tokernomics",
    "html_url": "https://github.com/tokernomics",
    "followers_url": "https://api.github.com/users/tokernomics/followers",
    "following_url": "https://api.github.com/users/tokernomics/following{/other_user}",
    "gists_url": "https://api.github.com/users/tokernomics/gists{/gist_id}",
    "starred_url": "https://api.github.com/users/tokernomics/starred{/owner}{/repo}",
    "subscriptions_url": "https://api.github.com/users/tokernomics/subscriptions",
    "organizations_url": "https://api.github.com/users/tokernomics/orgs",
    "repos_url": "https://api.github.com/users/tokernomics/repos",
    "events_url": "https://api.github.com/users/tokernomics/events{/privacy}",
    "received_events_url": "https://api.github.com/users/tokernomics/received_events",
    "type": "User",
    "site_admin": false
  },
  "committer": {
    "login": "web-flow",
    "id": 19864447,
    "node_id": "MDQ6VXNlcjE5ODY0NDQ3",
    "avatar_url": "https://avatars.githubusercontent.com/u/19864447?v=4",
    "gravatar_id": "",
    "url": "https://api.github.com/users/web-flow",
    "html_url": "https://github.com/web-flow",
    "followers_url": "https://api.github.com/users/web-flow/followers",
    "following_url": "https://api.github.com/users/web-flow/following{/other_user}",
    "gists_url": "https://api.github.com/users/web-flow/gists{/gist_id}",
    "starred_url": "https://api.github.com/users/web-flow/starred{/owner}{/repo}",
    "subscriptions_url": "https://api.github.com/users/web-flow/subscriptions",
    "organizations_url": "https://api.github.com/users/web-flow/orgs",
    "repos_url": "https://api.github.com/users/web-flow/repos",
    "events_url": "https://api.github.com/users/web-flow/events{/privacy}",
    "received_events_url": "https://api.github.com/users/web-flow/received_events",
    "type": "User",
    "site_admin": false
  },
  "parents": [ /* ... */ ],
  // ...
}

The data here more closely matches the Author struct, and not the GitUser struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant in the reply ["commit"]["author"] is a GitUser. In the reply ["author"] is indeed an Author.

The problem is that, apparently the API incorrectly returns an empty dict instead of null, when the author should be null?

See the schema:

    "author": {
      "anyOf": [
        {
          "type": "null"
        },
        {
          "title": "Simple User",
          "description": "A GitHub user.",
...

This seems like a GitHub bug, given that their docs claim otherwise than the reply and in the past the endpoint worked fine, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not familiar enough with the GitHub API to judge. I'm just going off this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like it's been broken for a while: https://github.com/orgs/community/discussions/57497

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think they monitor the discussions. It can be reported here: https://github.com/github/docs/issues

@SamWilsn
Copy link
Contributor Author

Seems like GitHub hasn't gotten around to fixing either the documentation or the API endpoint. How would you like to proceed?

@maflcko
Copy link
Contributor

maflcko commented Jul 18, 2024

I think they did fix the docs. For me it now says:

        {
          "title": "Empty Object",
          "description": "An object without any properties.",
          "type": "object",
          "properties": {},
          "additionalProperties": false
        }

Would it be possible to write code to conditionally deserialize into either Author (with fields), or into an empty dictionary (without any fields)?

@SamWilsn
Copy link
Contributor Author

SamWilsn commented Jul 18, 2024

I think they did fix the docs.

I don't know how I missed that! I'm sorry.

Would it be possible to write code to conditionally deserialize into either Author (with fields), or into an empty dictionary (without any fields)?

I could do something like:

#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
struct Empty {
}

#[derive(Deserialize)]
#[serde(untagged)]
enum MaybeAuthor {
    Empty(Empty),
    Author(Author),
}

I think I'd prefer mapping {} to None though, unless there's a good reason not to?

@maflcko
Copy link
Contributor

maflcko commented Jul 18, 2024

I think I'd prefer mapping {} to None though, unless there's a good reason not to?

Sure, I'd be ok with that as well.

@XAMPPRocky
Copy link
Owner

I think it would be better to have it be an option, if it's empty it should be considered equivalent to null.

@SamWilsn
Copy link
Contributor Author

@XAMPPRocky @maflcko Sorry for the super long delay on this one. Noticed you've added support for pr commits! I've co-opted this pull request into adding support for deserializing {}.

Hopefully I've followed the specific pr style well enough!

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.

lgtm

@maflcko
Copy link
Contributor

maflcko commented Aug 30, 2024

Needs rebase

@SamWilsn
Copy link
Contributor Author

Done

@XAMPPRocky
Copy link
Owner

Thank you for your PR, and congrats on your first contribution! 🎉

@XAMPPRocky XAMPPRocky merged commit 01767f3 into XAMPPRocky:main Sep 1, 2024
11 checks passed
@github-actions github-actions bot mentioned this pull request Aug 30, 2024
@SamWilsn SamWilsn deleted the pr-commits branch October 17, 2024 01:22
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