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

Paper over a few edge cases in RepoCommits #677

Closed
wants to merge 2 commits into from

Conversation

rossdylan
Copy link

@rossdylan rossdylan commented Aug 9, 2024

Despite the API docs saying this is a required field, it is possible to make commits using the api that have an author or committer field that does not have the login field.
Additionally, github deserializes an empty authors field in two different ways. Sometimes it is null and sometimes it is {}. I've added a generic helper that can be added to fields where this comes up as needed.

Despite the API docs saying this is a required field, it is possible to make
commits using the api that have no login field. There are prior cases where the
github rest api has just been wrong so this isn't that weird.
Another weird api spec incompatibility. I've found that some responses that are
missing an Authors field will either serialize it as `null` or `{}`. No idea why
@rossdylan rossdylan changed the title Make the login field of Author optional Paper over a few edge cases in RepoCommits Aug 9, 2024
@maflcko
Copy link
Contributor

maflcko commented Aug 9, 2024

Duplicate of #656 ?

@rossdylan
Copy link
Author

Duplicate of #656 ?

partially, I only made login optional since I haven't seen the others failing. Happy to rebase my changes onto that PR when it lands so we can also get the null/{} fixes too

@maflcko
Copy link
Contributor

maflcko commented Aug 10, 2024

Yeah, not sure where the other PR stands. I think the author needs to push the discussed adjustments, but the PR seems to have went stale for now.

@SamWilsn
Copy link
Contributor

So sorry about that! Just noticed this PR after updating mine. Apparently this is a popular API endpoint 🤣

@@ -403,7 +403,8 @@ pub struct IssuePullRequest {
#[derive(Debug, Clone, Hash, Eq, PartialEq, Serialize, Deserialize)]
#[non_exhaustive]
pub struct Author {
pub login: String,
#[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.

I don't think this is right, given that GH fixed the API upstream?

#656 looks correct (after a rebase/merge), instead.

Copy link
Author

Choose a reason for hiding this comment

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

yeah, I'll close this in favor of #656

@rossdylan rossdylan closed this Aug 30, 2024
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