Add endpoint that returns the latest stable release - #224
Conversation
Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Marek Kubica <marek@tarides.com>
|
I didn’t review the implementation but that’s the way to address #218 I was hoping for. 😄 |
| let newest_tagged bundles = | ||
| bundles | ||
| |> List.filter_map (fun bundle -> | ||
| match bundle.tag with | ||
| | None -> None | ||
| | Some tag -> | ||
| (match Scanf.sscanf_opt tag "%d.%d.%d" (fun x y z -> x, y, z) with | ||
| | None -> None | ||
| | Some tup -> Some (tup, bundle))) | ||
| |> List.sort (fun ((maj, min, patch), _) ((maj', min', patch'), _) -> | ||
| (* reverse sort, biggest first *) | ||
| match Int.compare maj' maj with | ||
| | 0 -> | ||
| (match Int.compare min' min with | ||
| | 0 -> Int.compare patch' patch | ||
| | otherwise -> otherwise) | ||
| | otherwise -> otherwise) | ||
| |> function | ||
| | [] -> None | ||
| | (_, bundle) :: _ -> Some bundle | ||
| ;; |
There was a problem hiding this comment.
I find the logic here a bit convoluted to follow. It's partially just due to the nesting and branching and piping together, and reverse sorting, but mainly just due to my limited cognitive capacities.
Still, with a just a few more lines of code, and a few auxiliary functions, I think this could be made more readable and more efficient (just requiring a single pass at the list and avoiding the need to allocate new lists or do sorting). E.g., with something like
| let newest_tagged bundles = | |
| bundles | |
| |> List.filter_map (fun bundle -> | |
| match bundle.tag with | |
| | None -> None | |
| | Some tag -> | |
| (match Scanf.sscanf_opt tag "%d.%d.%d" (fun x y z -> x, y, z) with | |
| | None -> None | |
| | Some tup -> Some (tup, bundle))) | |
| |> List.sort (fun ((maj, min, patch), _) ((maj', min', patch'), _) -> | |
| (* reverse sort, biggest first *) | |
| match Int.compare maj' maj with | |
| | 0 -> | |
| (match Int.compare min' min with | |
| | 0 -> Int.compare patch' patch | |
| | otherwise -> otherwise) | |
| | otherwise -> otherwise) | |
| |> function | |
| | [] -> None | |
| | (_, bundle) :: _ -> Some bundle | |
| ;; | |
| let compare_versions (maj, min, patch) (maj', min', patch') = | |
| match Int.compare maj maj' with | |
| | 0 -> | |
| (match Int.compare min min' with | |
| | 0 -> Int.compare patch patch' | |
| | otherwise -> otherwise) | |
| | otherwise -> otherwise | |
| let bundle_with_version b = | |
| let* tag = b.tag in | |
| let* v = parse_tag tag in | |
| Some (v, b) | |
| let max_bundle_version a b = | |
| match a, b with | |
| | None, o | o, None -> o | |
| | Some a, Some b -> | |
| if compare_versions (fst a) (fst b) > -1 then | |
| Some a | |
| else | |
| Some b | |
| let newest_tagged bundles = | |
| bundles | |
| |> List.fold_left (fun latest b -> | |
| b | |
| |> bundle_with_version | |
| |> max_bundle_version latest) | |
| None | |
| |> Option.map snd |
I'd not recommend this just to optimize in a place where it probably won't matter much, but since I also find something like this easier to reason about, I felt it worth recommending.
Still, this is just a suggestion on my part on not a blocking change request.
|
I notice there are no tests added with this. Should it have some unit tests for the new function? |
The project has not tests whatsoever; should I add a unit testing facility as part of this PR? |
Signed-off-by: Marek Kubica <marek@tarides.com>
I see! Ideally, the right time to fix this kind of oversight is as soon as its discovered, but since this is the last bit of work in our arc and that would add a lot of stuff not related to this PR, I'm OK with not addressing it here. If we have to do anything else of substance on this repo, we should start with a PR to add a test harness probably. LGTM! |
This adds a new endpoint (or a special case for an existing one if you want to see it that way) where it redirects to the latest stable release, without specifying which one that is.
Turns out the
installscript does not need to change as the release name "latest" can just be used as a version and it seems to make decent sense there.I've tested it locally that the server returns a redirect to 3.21.1 when requesting latest as well as testing the script in a Docker container running Alpine linux that it is able to install Dune using the
--release latestargument.Closes #218.