search

XAMPPRocky / octocrab

A modern, extensible GitHub API Client for Rust.
Other
1.02k stars 253 forks source link

Support pulls/{number}/commits #656

Open SamWilsn opened 1 month ago

XAMPPRocky commented 1 month ago

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

SamWilsn commented 1 month ago

Done!

SamWilsn commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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

XAMPPRocky commented 1 month ago

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

SamWilsn commented 1 month ago

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

SamWilsn commented 4 days ago

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

maflcko commented 4 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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