Remove dependency on base and stdio
#138
Conversation
|
You mentioned is too strict. Should we relax this then? What's the minimal required version to compile this library? |
lib/rule.ml
Outdated
| match t with | ||
| | [] -> None | ||
| | v :: vs -> Some (List.fold_left (fun a b -> if compare b a > 0 then b else a) v vs) in | ||
| let is_prefix prefix = Stre.starts_with filename prefix in |
There was a problem hiding this comment.
One thing I noticed while doing this is that in monorobot.opam the minimum required version for ocaml is >=4.08.0. There are some API changes in later versions that would make life easier in some places, like String.starts_with (here) and String.fold_left (lib/mrkdwn.ml line 11). Right now they're replaced with equivalents from Extlib and Devkit. Should we bump the minimum version up to 4.14? I think that's the compiler installed by default when running opam switch create ./ anyway.
I think |
| @@ -12,38 +11,38 @@ let log = Log.from "action" | |||
|
|
|||
| module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct | |||
| let partition_push (cfg : Config_t.config) n = | |||
| let default = Option.to_list cfg.prefix_rules.default_channel in | |||
| let default = Stdlib.Option.to_list cfg.prefix_rules.default_channel in | |||
There was a problem hiding this comment.
Stdlib is not required since open Base is removed. There are also a few more occurrences in the code base.
There was a problem hiding this comment.
I think Extlib somehow overrides Stdlib here, it doesn't compile unless I add this.
lib/common.ml
Outdated
| let empty () = Hashtbl.create (module String) | ||
| let to_list (l : 'a t) : (string * 'a) list = Hashtbl.to_alist l | ||
| let of_list (m : (string * 'a) list) : 'a t = Hashtbl.of_alist_exn (module String) m | ||
| let empty () = Hashtbl.create 0 |
There was a problem hiding this comment.
Let's assign it a non-zero value:
from https://v2.ocaml.org/releases/4.14/api/Hashtbl.html
Hashtbl.create n creates a new, empty hash table, with initial size n. For best results, n should be on the order of the expected number of elements that will be in the table. The table grows as needed, so n is just an initial guess.
There was a problem hiding this comment.
I'm not sure how big the initial hash table should be - it's being used to configs for different repos?
There was a problem hiding this comment.
A small number, e.g. 10, will do. Although interestingly base uses 0 (ref).
There was a problem hiding this comment.
If I understand correctly the Stringtbl is used to store per monorepo configs. Unless it's expected that we monitor multiple monorepos.
|
|
||
| type 'a t = 'a Hashtbl.M(String).t |
There was a problem hiding this comment.
Hashtbl also has a functorial interface like Map, so we can include Hashtbl.Make(String) too.
(also I'm not sure why we need Set, Hashtbl and Map in one repo. Functionally just Hashtbl will work, but that's out of the scope for this PR)
There was a problem hiding this comment.
Hashtbl.Make requires defining a hash module for string I believe: https://v2.ocaml.org/api/Hashtbl.HashedType.html
| @@ -157,13 +159,13 @@ let populate_commit ?(include_changes = true) repository (api_commit : api_commi | |||
| but looks like slack doesn't provide such functionality | |||
| *) | |||
| try | |||
| match String.split commit.author.date ~on:'T' with | |||
| match String.split_on_char 'T' commit.author.date with | |||
There was a problem hiding this comment.
this looks like Devkit.Time time format parsing, should be moved to devkit
There was a problem hiding this comment.
I'm guessing this is a change for a later PR?
lib/rule.ml
Outdated
| match t with | ||
| | [] -> None | ||
| | v :: vs -> Some (List.fold_left (fun a b -> if compare b a > 0 then b else a) v vs) in | ||
| let is_prefix prefix = Stre.starts_with filename prefix in |
lib/common.ml
Outdated
| with exn -> fmt_error "%s" (Exn.to_string exn) | ||
|
|
||
| let longest_common_prefix xs = | ||
| match xs with | ||
| | [] -> "" | ||
| | [ x ] -> x | ||
| | x :: _ -> String.sub x ~pos:0 ~len:(Stre.common_prefix x (List.sort xs ~compare:String.compare |> List.last_exn)) | ||
| | x :: _ -> String.sub x 0 (Stre.common_prefix x (List.sort String.compare xs |> List.rev |> List.hd)) |
There was a problem hiding this comment.
| | x :: _ -> String.sub x 0 (Stre.common_prefix x (List.sort String.compare xs |> List.rev |> List.hd)) | |
| | x :: xs -> | |
| let longest = List.sort (Fun.flip String.compare) xs |> List.hd in | |
| String.sub x 0 (Stre.common_prefix x longest) |
less code-golfing, more clarity :)
There was a problem hiding this comment.
There's a problem with the original code's logic here. Got the following
utop # longest_common_prefix ["/test/paths/not_here"; "/test/path/here"; "/test/paths/here"];;
- : string = "/test/paths/not_here"
I'll fix it in another PR.
Description of the task
Removed and replaced all uses of the Base and Stdio libraries.
How to test
Should compile without issues and the tests should pass.