From 9a132212639edc37b9e458c698f97fcebaad1bd2 Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Wed, 8 Aug 2018 14:41:11 +0300
Subject: [PATCH] Clean up the error handling for lib names

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 src/jbuild.ml                                 | 40 +++++++++----------
 .../test-cases/no-name-field/run.t            |  7 ++--
 2 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/src/jbuild.ml b/src/jbuild.ml
index e3911ec4..8c0eb18d 100644
--- a/src/jbuild.ml
+++ b/src/jbuild.ml
@@ -45,36 +45,43 @@ module Lib_name : sig
     | Warn of t
     | Invalid
 
-  val error_message : string
-
-  val warn_message : string
+  val invalid_message : string
 
   val to_string : t -> string
 
   val of_string : string -> result
 
+  val validate : (Loc.t * result) -> wrapped:bool -> t
+
   val t : (Loc.t * result) Sexp.Of_sexp.t
 end = struct
   type t = string
 
-  let error_message =
+  let invalid_message =
     "invalid library name.\n\
      Hint: library names must be non-empty and composed only of \
      the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'"
 
-  let warn_message =
+  let wrapped_message =
     sprintf
       "%s.\n\
        This is temporary allowed for libraries with (wrapped false).\
        \nIt will not be supported in the future. \
        Please choose a valid name field."
-      error_message
+      invalid_message
 
   type result =
     | Ok of t
     | Warn of t
     | Invalid
 
+  let validate (loc, res) ~wrapped =
+    match res, wrapped with
+    | Ok s, _ -> s
+    | Warn _, true -> Loc.fail loc "%s" wrapped_message
+    | Warn s, false -> Loc.warn loc "%s" wrapped_message; s
+    | Invalid, _ -> Loc.fail loc "%s" invalid_message
+
   let valid_char = function
     | 'A'..'Z' | 'a'..'z' | '_' | '0'..'9' -> true
     | _ -> false
@@ -952,29 +959,20 @@ module Library = struct
        in
        let name =
          match name, public with
-         | Some (loc, n), _ ->
-           begin match n, wrapped with
-           | Ok n, _ ->  n
-           | Warn _, true -> Loc.fail loc "%s" Lib_name.error_message
-           | Warn n, false -> Loc.warn loc "%s" Lib_name.warn_message; n
-           | Invalid, _ -> Loc.fail loc "%s" Lib_name.error_message
-           end
+         | Some n, _ ->
+           Lib_name.validate n ~wrapped
            |> Lib_name.to_string
          | None, Some { name = (loc, name) ; _ }  ->
            if dune_version >= (1, 1) then
-             match Lib_name.of_string name, wrapped with
-             | Ok n, _ -> Lib_name.to_string n
-             | Warn _, false ->
-               Loc.warn loc "%s" Lib_name.warn_message;
-               name
-             | Warn _, true
-             | Invalid, _ ->
+             match Lib_name.of_string name with
+             | Ok m -> Lib_name.to_string m
+             | Warn _ | Invalid ->
                of_sexp_errorf loc
                  "%s.\n\
                   Public library names don't have this restriction. \
                   You can either change this public name to be a valid library \
                   name or add a \"name\" field with a valid library name."
-                 Lib_name.error_message
+                 Lib_name.invalid_message
            else
              of_sexp_error loc "name field cannot be omitted before version \
                                 1.1 of the dune language"
diff --git a/test/blackbox-tests/test-cases/no-name-field/run.t b/test/blackbox-tests/test-cases/no-name-field/run.t
index e35fbc5d..a25f890b 100644
--- a/test/blackbox-tests/test-cases/no-name-field/run.t
+++ b/test/blackbox-tests/test-cases/no-name-field/run.t
@@ -33,8 +33,7 @@ it's just a warning
   $ dune build --root public-name-invalid-wrapped-false
   Info: creating file dune-project with this contents: (lang dune 1.1)
   File "dune", line 3, characters 14-21:
-  Warning: invalid library name.
+  Error: invalid library name.
   Hint: library names must be non-empty and composed only of the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'.
-  This is temporary allowed for libraries with (wrapped false).
-  It will not be supported in the future. Please choose a valid name field.
-  Entering directory 'public-name-invalid-wrapped-false'
+  Public library names don't have this restriction. You can either change this public name to be a valid library name or add a "name" field with a valid library name.
+  [1]