tests, api, slack_messages: standardize github cache file for testing, and cleaned up populate compare unfurling

Author: dombong

lib/action.ml

@@ -337,8 +337,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
         | Ok commit -> Lwt.return_some @@ (link, Slack_message.populate_commit repo commit)
         )
       | Compare (repo, (base, merge)) ->
-        let basehead = Printf.sprintf "%s...%s" base merge in
-        ( match%lwt Github_api.get_compare ~ctx ~repo ~basehead with
+        ( match%lwt Github_api.get_compare ~ctx ~repo ~basehead:(base, merge) with
         | Error _ -> Lwt.return_none
         | Ok compare -> Lwt.return_some @@ (link, Slack_message.populate_compare repo compare)
         )

lib/api.ml

@@ -8,7 +8,7 @@ module type Github = sig
   val get_api_commit : ctx:Context.t -> repo:repository -> sha:string -> (api_commit, string) Result.t Lwt.t
   val get_pull_request : ctx:Context.t -> repo:repository -> number:int -> (pull_request, string) Result.t Lwt.t
   val get_issue : ctx:Context.t -> repo:repository -> number:int -> (issue, string) Result.t Lwt.t
-  val get_compare : ctx:Context.t -> repo:repository -> basehead:string -> (compare, string) Result.t Lwt.t
+  val get_compare : ctx:Context.t -> repo:repository -> basehead:Github.basehead -> (compare, string) Result.t Lwt.t
   val get_release_tag : ctx:Context.t -> repo:repository -> release_tag:string -> (release_tag, string) Result.t Lwt.t
 
   val request_reviewers

lib/api_local.ml

@@ -8,50 +8,50 @@ let cache_dir = Caml.Filename.concat cwd "github-api-cache"
 
 (** return the file with a function f applied unless the file is empty;
  empty file:this is needed to simulate 404 returns from github *)
-let get_cache_file_f url f =
+let with_cache_file url f =
   match get_local_file url with
   | Error e ->
     let err_msg = sprintf "error while getting local file: %s\ncached for url: %s" e url in
     Stdio.print_endline err_msg;
-    Lwt.return @@ Error err_msg
-  | Ok "" -> Lwt.return @@ Error "empty file"
-  | Ok file -> Lwt.return @@ Ok (f file)
+    Lwt.return_error err_msg
+  | Ok "" -> Lwt.return_error "empty file"
+  | Ok file -> Lwt.return_ok (f file)
 
 let clean_forward_slashes = String.substr_replace_all ~pattern:"/" ~with_:"_"
 
+(** get a member of the repo cached API call providing
+the member kind (pull, issue, commit, compare, etc),
+_ref (pr number, issue number, commit sha, compare basehead, etc),
+and its Github_j.<kind>_of_string function.
+NB: please save the cache file in the same format *)
+let get_repo_member_cache ~(repo : Github_t.repository) ~kind ~_ref ~of_string =
+  let file = clean_forward_slashes (sprintf "%s_%s_%s" repo.full_name kind _ref) in
+  let url = Caml.Filename.concat cache_dir file in
+  with_cache_file url of_string
+
 module Github : Api.Github = struct
   let get_config ~(ctx : Context.t) ~repo:_ =
     let url = Caml.Filename.concat cwd ctx.config_filename in
-    get_cache_file_f url Config_j.config_of_string
+    with_cache_file url Config_j.config_of_string
 
   let get_branch ~ctx:_ ~(repo : Github_t.repository) ~name =
-    let repo_branch = clean_forward_slashes (sprintf "%s_branch_%s" repo.full_name name) in
-    let url = Caml.Filename.concat cache_dir repo_branch in
-    get_cache_file_f url Github_j.branch_of_string
+    get_repo_member_cache ~repo ~kind:"branch" ~_ref:name ~of_string:Github_j.branch_of_string
 
-  let get_api_commit ~ctx:_ ~repo:_ ~sha =
-    let url = Caml.Filename.concat cache_dir sha in
-    get_cache_file_f url Github_j.api_commit_of_string
+  let get_api_commit ~ctx:_ ~repo ~sha =
+    get_repo_member_cache ~repo ~kind:"commit" ~_ref:sha ~of_string:Github_j.api_commit_of_string
 
   let get_pull_request ~ctx:_ ~(repo : Github_t.repository) ~number =
-    let pr = clean_forward_slashes (sprintf "%s_pull_%d" repo.full_name number) in
-    let url = Caml.Filename.concat cache_dir pr in
-    get_cache_file_f url Github_j.pull_request_of_string
+    get_repo_member_cache ~repo ~kind:"pull" ~_ref:(Int.to_string number) ~of_string:Github_j.pull_request_of_string
 
   let get_issue ~ctx:_ ~(repo : Github_t.repository) ~number =
-    let issue = clean_forward_slashes (sprintf "%s_issue_%d" repo.full_name number) in
-    let url = Caml.Filename.concat cache_dir issue in
-    get_cache_file_f url Github_j.issue_of_string
+    get_repo_member_cache ~repo ~kind:"issue" ~_ref:(Int.to_string number) ~of_string:Github_j.issue_of_string
 
-  let get_compare ~ctx:_ ~(repo : Github_t.repository) ~basehead =
-    let compare = clean_forward_slashes (sprintf "%s_compare_%s" repo.full_name basehead) in
-    let url = Caml.Filename.concat cache_dir compare in
-    get_cache_file_f url Github_j.compare_of_string
+  let get_compare ~ctx:_ ~(repo : Github_t.repository) ~basehead:(base, merge) =
+    get_repo_member_cache ~repo ~kind:"compare" ~_ref:(sprintf "%s...%s" base merge)
+      ~of_string:Github_j.compare_of_string
 
   let get_release_tag ~ctx:_ ~(repo : Github_t.repository) ~release_tag =
-    let release_tag = clean_forward_slashes (sprintf "%s_release_tag_%s" repo.full_name release_tag) in
-    let url = Caml.Filename.concat cache_dir release_tag in
-    get_cache_file_f url Github_j.release_tag_of_string
+    get_repo_member_cache ~repo ~kind:"release_tag" ~_ref:release_tag ~of_string:Github_j.release_tag_of_string
 
   let request_reviewers ~ctx:_ ~repo:_ ~number:_ ~reviewers:_ = Lwt.return @@ Error "undefined for local setup"
 end

lib/api_remote.ml

@@ -19,8 +19,8 @@ module Github : Api.Github = struct
   let branches_url ~(repo : Github_t.repository) ~name =
     String.substr_replace_first ~pattern:"{/branch}" ~with_:(sprintf "/%s" name) repo.branches_url
 
-  let compare_url ~(repo : Github_t.repository) ~basehead =
-    String.substr_replace_first ~pattern:"{/basehead}" ~with_:(sprintf "/%s" basehead) repo.compare_url
+  let compare_url ~(repo : Github_t.repository) ~basehead:(base, merge) =
+    String.substr_replace_first ~pattern:"{/basehead}" ~with_:(sprintf "/%s...%s" base merge) repo.compare_url
 
   let releases_tags_url ~(repo : Github_t.repository) ~release_tag =
     String.substr_replace_first ~pattern:"{/release_tag}" ~with_:(sprintf "/tags/%s" release_tag) repo.releases_url
@@ -88,7 +88,6 @@ module Github : Api.Github = struct
     let%lwt res =
       compare_url ~repo ~basehead |> get_resource ~secrets:(Context.get_secrets_exn ctx) ~repo_url:repo.url
     in
-
     Lwt.return @@ Result.map res ~f:Github_j.compare_of_string
 
   let get_release_tag ~(ctx : Context.t) ~(repo : Github_t.repository) ~release_tag =

lib/github.atd

@@ -288,7 +288,7 @@ type api_commit = {
   sha: commit_hash;
   commit: inner_commit;
   html_url <ocaml name="url"> : string;
-  ?author: github_user option;
+  ?author: github_user option; (* will be none if author's email is not associated with any github account *)
   ~files <ocaml default="[]">: file list;
   ~stats <ocaml default="{total=0; additions=0; deletions=0;}">: api_commit_stats;
 }

lib/github.ml

@@ -151,10 +151,7 @@ let gh_link_of_string url_str =
         match Re2.find_submatches_exn compare_basehead_re basehead with
         | [| _; Some base; _; Some merge |] -> Some (Compare (repo, (base, merge)))
         | _ -> None
-      with e ->
-        let msg = Exn.to_string e in
-        Stdio.printf "there was an error: %s\n" msg;
-        None
+      with Re2.Exceptions.Regex_match_failed _ -> None
     in
     begin
       try

lib/slack_message.ml

@@ -145,7 +145,7 @@ let condense_file_changes files =
     in
     if String.is_empty prefix_path then "" else sprintf " in `%s/`" prefix_path
 
-let populate_commit ?(for_compare = false) repository (commit : api_commit) =
+let populate_commit ?(include_changes = true) repository (commit : api_commit) =
   let ({ sha; commit; url; author; files; _ } : api_commit) = commit in
   let title =
     match author with
@@ -158,7 +158,7 @@ let populate_commit ?(for_compare = false) repository (commit : api_commit) =
         (escape_mrkdwn @@ first_line commit.message)
         (escape_mrkdwn commit.author.name)
   in
-  let changes =
+  let changes () =
     let where = condense_file_changes files in
     let when_ =
       (*
@@ -184,9 +184,9 @@ let populate_commit ?(for_compare = false) repository (commit : api_commit) =
     sprintf "modified %d files%s %s" (List.length files) where when_
   in
   let text =
-    match for_compare with
-    | true -> sprintf "%s\n" title
-    | false -> sprintf "%s\n%s" title changes
+    match include_changes with
+    | false -> sprintf "%s\n" title
+    | true -> sprintf "%s\n%s" title (changes ())
   in
   let fallback = sprintf "[%s] %s - %s" (Slack.git_short_sha_hash sha) commit.message commit.author.name in
   {
@@ -212,55 +212,34 @@ let populate_commit ?(for_compare = false) repository (commit : api_commit) =
   }
 
 let populate_compare repository (compare : compare) =
-  if compare.total_commits = 0 then (
-    let no_commit_msg = "There are no commit difference in this compare!" in
+  let base =
     {
       (base_attachment repository) with
       footer = Some (simple_footer repository);
       author_icon = None;
       color = Some Colors.gray;
       mrkdwn_in = Some [ "text" ];
-      text = Some no_commit_msg;
-      fallback = Some no_commit_msg;
+      text = None;
+      fallback = None;
     }
-  )
-  else (
-    let commits_unfurl = List.map compare.commits ~f:(populate_commit ~for_compare:true repository) in
+  in
+  match compare.total_commits = 0 with
+  | true ->
+    let no_commit_msg = "There are no commit difference in this compare!" in
+    { base with text = Some no_commit_msg; fallback = Some no_commit_msg }
+  | false ->
+    let commits_unfurl = List.map compare.commits ~f:(populate_commit ~include_changes:false repository) in
     let commits_unfurl_text =
-      List.map commits_unfurl ~f:(fun commit_unfurl ->
-        match commit_unfurl.text with
-        | Some text -> text
-        | None -> ""
-      )
+      List.map commits_unfurl ~f:(fun commit_unfurl -> Option.value commit_unfurl.text ~default:"")
     in
-
     let commits_unfurl_fallback =
-      List.map commits_unfurl ~f:(fun commit_unfurl ->
-        match commit_unfurl.fallback with
-        | Some fallback -> fallback
-        | None -> ""
-      )
+      List.map commits_unfurl ~f:(fun commit_unfurl -> Option.value commit_unfurl.fallback ~default:"")
     in
     let file_stats =
       match condense_file_changes compare.files with
       | "" -> ""
       | text -> sprintf "\n\n%s" text
     in
-    let repo_text = sprintf "<%s|[%s]>" (escape_mrkdwn repository.url) (escape_mrkdwn repository.name) in
-    let total_commits_text =
-      if compare.total_commits > 1 then
-        sprintf "<%s|%d _commits_>" (escape_mrkdwn compare.html_url) compare.total_commits
-      else sprintf "<%s|%d _commit_>" (escape_mrkdwn compare.html_url) compare.total_commits
-    in
-    let text = sprintf "%s %s:\n%s%s" repo_text total_commits_text (String.concat commits_unfurl_text) file_stats in
+    let text = sprintf "%s%s" (String.concat commits_unfurl_text) file_stats in
     let fallback = String.concat commits_unfurl_fallback in
-    {
-      (base_attachment repository) with
-      footer = Some (simple_footer repository);
-      author_icon = None;
-      color = Some Colors.gray;
-      mrkdwn_in = Some [ "text" ];
-      text = Some text;
-      fallback = Some fallback;
-    }
-  )
+    { base with text = Some text; fallback = Some fallback }

test/github-api-cache/0d95302addd66c1816bce1b1d495ed1c93ccd478 renamed to test/github-api-cache/ahrefs_monorepo_commit_0d95302addd66c1816bce1b1d495ed1c93ccd478

@@ -0,0 +1,79 @@
+{
+  "sha": "0d95302addd66c1816bce1b1d495ed1c93ccd478",
+  "node_id": "00000000000000000000",
+  "commit": {
+    "author": {
+      "name": "Louis",
+      "email": "[email protected]",
+      "date": "2020-06-02T03:14:51Z"
+    },
+    "committer": {
+      "name": "GitHub Enterprise",
+      "email": "[email protected]",
+      "date": "2020-06-02T03:14:51Z"
+    },
+    "message": "Update README.md",
+    "tree": {
+      "sha": "ee5c539cad37c77348ce7a55756acc542b41cfc7",
+      "url": "https://github.com/api/v3/repos/ahrefs/monorepo/git/trees/ee5c539cad37c77348ce7a55756acc542b41cfc7"
+    },
+    "url": "https://github.com/api/v3/repos/ahrefs/monorepo/git/commits/0d95302addd66c1816bce1b1d495ed1c93ccd478",
+    "comment_count": 0,
+    "verification": {
+      "verified": false,
+      "reason": "unsigned",
+      "signature": null,
+      "payload": null
+    }
+  },
+  "url": "https://github.com/api/v3/repos/ahrefs/monorepo/commits/0d95302addd66c1816bce1b1d495ed1c93ccd478",
+  "html_url": "https://github.com/ahrefs/monorepo/commit/0d95302addd66c1816bce1b1d495ed1c93ccd478",
+  "comments_url": "https://github.com/api/v3/repos/ahrefs/monorepo/commits/0d95302addd66c1816bce1b1d495ed1c93ccd478/comments",
+  "author": {
+    "login": "Khady",
+    "id": 0,
+    "node_id": "00000000000000000000",
+    "avatar_url": "https://github.com/avatars/u/0",
+    "gravatar_id": "",
+    "url": "https://github.com/api/v3/users/Khady",
+    "html_url": "https://github.com/Khady",
+    "followers_url": "https://github.com/api/v3/users/Khady/followers",
+    "following_url": "https://github.com/api/v3/users/Khady/following{/other_user}",
+    "gists_url": "https://github.com/api/v3/users/Khady/gists{/gist_id}",
+    "starred_url": "https://github.com/api/v3/users/Khady/starred{/owner}{/repo}",
+    "subscriptions_url": "https://github.com/api/v3/users/Khady/subscriptions",
+    "organizations_url": "https://github.com/api/v3/users/Khady/orgs",
+    "repos_url": "https://github.com/api/v3/users/Khady/repos",
+    "events_url": "https://github.com/api/v3/users/Khady/events{/privacy}",
+    "received_events_url": "https://github.com/api/v3/users/Khady/received_events",
+    "type": "User",
+    "site_admin": false
+  },
+  "committer": null,
+  "parents": [
+    {
+      "sha": "04cb72d6dc8d92131282a7eff57f6caf632f0a39",
+      "url": "https://github.com/api/v3/repos/ahrefs/monorepo/commits/04cb72d6dc8d92131282a7eff57f6caf632f0a39",
+      "html_url": "https://github.com/ahrefs/monorepo/commit/04cb72d6dc8d92131282a7eff57f6caf632f0a39"
+    }
+  ],
+  "stats": {
+    "total": 2,
+    "additions": 1,
+    "deletions": 1
+  },
+  "files": [
+    {
+      "sha": "e7d353f786a2f29d0cf7ed6fdad08ec446c1b9b1",
+      "filename": "README.md",
+      "status": "modified",
+      "additions": 1,
+      "deletions": 1,
+      "changes": 2,
+      "blob_url": "https://github.com/ahrefs/monorepo/blob/0d95302addd66c1816bce1b1d495ed1c93ccd478/README.md",
+      "raw_url": "https://github.com/ahrefs/monorepo/raw/0d95302addd66c1816bce1b1d495ed1c93ccd478/README.md",
+      "contents_url": "https://github.com/api/v3/repos/ahrefs/monorepo/contents/README.md?ref=0d95302addd66c1816bce1b1d495ed1c93ccd478",
+      "patch": ""
+    }
+  ]
+}