Support pulls/{number}/commits

[SamWilsn]

[XAMPPRocky]

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

[SamWilsn]

Done!

[SamWilsn]

@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]

@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]

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

[XAMPPRocky]

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

[SamWilsn]

It's, uh... mostly Option now :sweat_smile:

[SamWilsn]

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

[maflcko]

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]

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]

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]

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

[SamWilsn]

@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!

[maflcko]

Needs rebase

[SamWilsn]

Done

[XAMPPRocky]

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