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

Gists API: Complete support #371

Merged
merged 6 commits into from
May 21, 2023
Merged

Conversation

envp
Copy link
Contributor

@envp envp commented May 20, 2023

This PR completes support for GitHub's gists api.

The remaining 3 endpoints in #27 (comment) will all be implemented here:

  • List gists for a user (GET /users/{username}/gists)
  • List all public gists (GET /gists)
  • List public gists (GET /gists/public)

Closes #27

@envp envp force-pushed the issue-27/gists-api branch 6 times, most recently from df6d26a to c18ac2b Compare May 20, 2023 20:10

/// Handles query data for the `GET /gists/public` endpoint.
#[derive(Debug, serde::Serialize)]
pub struct ListAllPublicGistsBuilder<'octo> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is significant overlap between ListAllPublicGistsBuilder and ListAllGistsBuilder.

An alternative implementation I considered was to have a flag in ListAllGistsBuilder decide which endpoint to call, but decided against it because GET /gists/public offers specific guarantees on the order of results returned.

I've no strong opinions on the structure otherwise, happy to merge these two types if that is considered more desirable.

Copy link
Owner

Choose a reason for hiding this comment

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

I think separate structs is probably best as it future proofs us from GitHub making them slightly different in the future.

Another approach you could have taken is to have a type parameter for the visibility, that way it would technically be two different types ListGistsBuilder<All> and ListGistsBuilder<Public> but they could share all of the same fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, will look into this!

@envp envp force-pushed the issue-27/gists-api branch from c18ac2b to 81d2f6d Compare May 20, 2023 20:25
@envp envp marked this pull request as ready for review May 20, 2023 21:30
src/api/users.rs Outdated
Comment on lines 46 to 50
pub fn list_user_gists(&self, username: impl AsRef<str>) -> ListGistsForUserBuilder<'octo> {
ListGistsForUserBuilder::new(self.crab, username.as_ref().to_string())
}
}
Copy link
Contributor Author

@envp envp May 20, 2023

Choose a reason for hiding this comment

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

Official docs file this endpoint with the rest of gist endpoints. Would you prefer I move this handler into api/gists.rs as well? @XAMPPRocky

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good to me.

envp added 4 commits May 20, 2023 17:55
Add `GistHandler::list_all_gists(...)` which:

* For an authenticated caller, this endpoint lists all gists of the
  authenticated user.

* If called anonymously, it will retrieve ALL public gists in GitHub.
  Use with caution!

There is partial overlap with functionality in
`CurrentAuthHandler::list_gists_for_authenticated_user`.
Adds the handler: `GistsHandler::list_all_recent_public_gists`

The struct introduced has significant overlap with `ListAllGistsBuilder`
but is kept as a separate type since the response for
`ListAllPublicGistsBuilder` has specific sorting guarantees which differ
from `GET /gists`
* Create module `api/users.rs`

* Add a handler for `GET /users/{username}/gists`
  This is technically documented under the [gists api][1], but is
  grouped with other functions for handling routes under `/users/`.

[1] https://docs.github.com/en/rest/gists/gists?apiVersion=2022-11-28#list-gists-for-a-user
@envp envp force-pushed the issue-27/gists-api branch from bb8245f to 0675c93 Compare May 20, 2023 21:55
Since both of these share almost all of the code except for the endpoint
we implement a trait with an associated constant that provides this
value.
@envp envp requested a review from XAMPPRocky May 20, 2023 22:44
@envp envp force-pushed the issue-27/gists-api branch from 490eca2 to 22d6acc Compare May 21, 2023 15:25
GitHub keeps this endpoint with other gist related endpoints so we will
do the same for minimizing surprise.
@envp envp force-pushed the issue-27/gists-api branch from 22d6acc to f637a8f Compare May 21, 2023 15:42
@XAMPPRocky
Copy link
Owner

Thank you for your PR!

@XAMPPRocky XAMPPRocky merged commit e0d5c47 into XAMPPRocky:main May 21, 2023
@github-actions github-actions bot mentioned this pull request May 21, 2023
@envp envp deleted the issue-27/gists-api branch May 21, 2023 20:28
@envp envp mentioned this pull request May 22, 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.

Add Gists API
2 participants