Skip to content

All commits#245

Merged
tmcgilchrist merged 2 commits into
mirage:masterfrom
Stevendeo:all-commits
Jul 19, 2021
Merged

All commits#245
tmcgilchrist merged 2 commits into
mirage:masterfrom
Stevendeo:all-commits

Conversation

@Stevendeo
Copy link
Copy Markdown
Contributor

Adding a 'get_all_commits' request

@avsm
Copy link
Copy Markdown
Member

avsm commented Jun 22, 2021

Thanks for the PR! I've pushed a merge commit to get the CI green on your branch.

@Stevendeo
Copy link
Copy Markdown
Contributor Author

Thanks!

@tmcgilchrist
Copy link
Copy Markdown
Member

@Stevendeo the linting issue is fixed on master if you want to merge again (or rebase).

Copy link
Copy Markdown
Member

@tmcgilchrist tmcgilchrist left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Stevendeo. I suggest you use the Stream module, comments inline.
Git rebase/merge/cherry-pick the branch and I can merge it.

The releases.ml test code uses a streamed listing of releases in a repository, as an example of how to consume s 'a Stream.t

Comment thread lib/github_s.mli Outdated
Comment on lines +885 to +889
val get_commits :
?token:Token.t ->
user:string -> repo:string ->
unit -> Github_t.commits Response.t Monad.t
(** [get_commits ~user ~repo ()] lists all commits in [user]/[repo]. *)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could be improved by using Stream.t since the response is paginated, the Stream module provides an abstraction over GitHub's paginated endpoints.

Suggested change
val get_commits :
?token:Token.t ->
user:string -> repo:string ->
unit -> Github_t.commits Response.t Monad.t
(** [get_commits ~user ~repo ()] lists all commits in [user]/[repo]. *)
val get_commits :
?token:Token.t ->
user:string -> repo:string ->
unit -> Github_t.commit Stream.t
(** [get_commits ~user ~repo ()] stream of all commits in [user]/[repo]. *)

Comment thread lib/github_core.ml Outdated
Comment on lines +1958 to +1960
let get_commits ?token ~user ~repo () =
let uri = URI.repo_commits ~user ~repo in
API.get ?token ~uri (fun b -> return (commits_of_string b))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using get_stream this should be something like

Suggested change
let get_commits ?token ~user ~repo () =
let uri = URI.repo_commits ~user ~repo in
API.get ?token ~uri (fun b -> return (commits_of_string b))
let get_commits ?token ~user ~repo () =
let uri = URI.repo_commits ~user ~repo in
let fail_handlers = [
API.code_handler
~expected_code:`Not_found
(fun _ -> Lwt.return [])
] in
API.get_stream ?token ~uri ~fail_handlers (fun b -> return (commits_of_string b))

@Stevendeo
Copy link
Copy Markdown
Contributor Author

Thanks for the review, I just fixed my commit with the Stream module

@tmcgilchrist tmcgilchrist merged commit bd3c62e into mirage:master Jul 19, 2021
tmcgilchrist added a commit to tmcgilchrist/opam-repository that referenced this pull request Jul 21, 2021
CHANGES:

- Fixes to odoc warnings and cohttp dependencies (@Aaylor mirage/ocaml-github#244)
- Support cohttp 4.0. (@tmcgilchrist mirage/ocaml-github#257)
- Support for 4.12 and fixing recent compiler warnings (@tmcgilchrist mirage/ocaml-github#246 mirage/ocaml-github#252 and @emillon mirage/ocaml-github#250 mirage/ocaml-github#247 mirage/ocaml-github#251)
- Add a new package `github-data` which contains just the serialisation logic
  without a dependency on the web stack (mirage/ocaml-github#248 @emillon)
- Add Github checks API support (mirage/ocaml-github#249 @tmcgilchrist)
- Label field missing in branch refs for ghost (@dra27 and @tmcgilchrist mirage/ocaml-github#256)
- Get all commits for user/repo (@Stevendeo mirage/ocaml-github#245)
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