Improve Diagnostic error message reporting

4 files changed, 107 insertions, 32 deletions

diff --git a/clap.zig b/clap.zig
index b7235f1..2eab860 100644
--- a/clap.zig
+++ b/clap.zig
@@ -289,27 +289,36 @@ pub fn Args(comptime Id: type, comptime params: []const Param(Id)) type {
 
 /// Optional diagnostics used for reporting useful errors
 pub const Diagnostic = struct {
-    name: Names,
+    arg: []const u8 = "",
+    name: Names = Names{},
 
     /// Default diagnostics reporter when all you want is English with no colors.
     /// Use this as a reference for implementing your own if needed.
     pub fn report(diag: Diagnostic, stream: anytype, err: anyerror) !void {
-        const prefix = if (diag.name.short) |_| "-" else "--";
-        const name = if (diag.name.short) |*c| @as(*const [1]u8, c)[0..] else diag.name.long.?;
+        const Arg = struct {
+            prefix: []const u8,
+            name: []const u8,
+        };
+        const a = if (diag.name.short) |*c|
+            Arg{ .prefix = "-", .name = @as(*const [1]u8, c)[0..] }
+        else if (diag.name.long) |l|
+            Arg{ .prefix = "--", .name = l }
+        else
+            Arg{ .prefix = "", .name = diag.arg };
 
         switch (err) {
-            error.DoesntTakeValue => try stream.print("The argument '{}{}' does not take a value\n", .{ prefix, name }),
-            error.MissingValue => try stream.print("The argument '{}{}' requires a value but none was supplied\n", .{ prefix, name }),
-            error.InvalidArgument => try stream.print("Invalid argument '{}{}'\n", .{ prefix, name }),
+            error.DoesntTakeValue => try stream.print("The argument '{}{}' does not take a value\n", .{ a.prefix, a.name }),
+            error.MissingValue => try stream.print("The argument '{}{}' requires a value but none was supplied\n", .{ a.prefix, a.name }),
+            error.InvalidArgument => try stream.print("Invalid argument '{}{}'\n", .{ a.prefix, a.name }),
             else => try stream.print("Error while parsing arguments: {}\n", .{@errorName(err)}),
         }
     }
 };
 
-fn testDiag(names: Names, err: anyerror, expected: []const u8) void {
+fn testDiag(diag: Diagnostic, err: anyerror, expected: []const u8) void {
     var buf: [1024]u8 = undefined;
     var slice_stream = io.fixedBufferStream(&buf);
-    (Diagnostic{ .name = names }).report(slice_stream.outStream(), err) catch unreachable;
+    diag.report(slice_stream.outStream(), err) catch unreachable;
 
     const actual = slice_stream.getWritten();
     if (!mem.eql(u8, actual, expected)) {
@@ -322,14 +331,16 @@ fn testDiag(names: Names, err: anyerror, expected: []const u8) void {
 }
 
 test "Diagnostic.report" {
-    testDiag(.{ .short = 'c' }, error.DoesntTakeValue, "The argument '-c' does not take a value\n");
-    testDiag(.{ .long = "cc" }, error.DoesntTakeValue, "The argument '--cc' does not take a value\n");
-    testDiag(.{ .short = 'c' }, error.MissingValue, "The argument '-c' requires a value but none was supplied\n");
-    testDiag(.{ .long = "cc" }, error.MissingValue, "The argument '--cc' requires a value but none was supplied\n");
-    testDiag(.{ .short = 'c' }, error.InvalidArgument, "Invalid argument '-c'\n");
-    testDiag(.{ .long = "cc" }, error.InvalidArgument, "Invalid argument '--cc'\n");
-    testDiag(.{ .short = 'c' }, error.SomethingElse, "Error while parsing arguments: SomethingElse\n");
-    testDiag(.{ .long = "cc" }, error.SomethingElse, "Error while parsing arguments: SomethingElse\n");
+    testDiag(.{ .arg = "c" }, error.InvalidArgument, "Invalid argument 'c'\n");
+    testDiag(.{ .name = .{ .long = "cc" } }, error.InvalidArgument, "Invalid argument '--cc'\n");
+    testDiag(.{ .name = .{ .short = 'c' } }, error.DoesntTakeValue, "The argument '-c' does not take a value\n");
+    testDiag(.{ .name = .{ .long = "cc" } }, error.DoesntTakeValue, "The argument '--cc' does not take a value\n");
+    testDiag(.{ .name = .{ .short = 'c' } }, error.MissingValue, "The argument '-c' requires a value but none was supplied\n");
+    testDiag(.{ .name = .{ .long = "cc" } }, error.MissingValue, "The argument '--cc' requires a value but none was supplied\n");
+    testDiag(.{ .name = .{ .short = 'c' } }, error.InvalidArgument, "Invalid argument '-c'\n");
+    testDiag(.{ .name = .{ .long = "cc" } }, error.InvalidArgument, "Invalid argument '--cc'\n");
+    testDiag(.{ .name = .{ .short = 'c' } }, error.SomethingElse, "Error while parsing arguments: SomethingElse\n");
+    testDiag(.{ .name = .{ .long = "cc" } }, error.SomethingElse, "Error while parsing arguments: SomethingElse\n");
 }
 
 /// Parses the command line arguments passed into the program based on an
diff --git a/clap/args.zig b/clap/args.zig
index 4d97017..52626fc 100644
--- a/clap/args.zig
+++ b/clap/args.zig
@@ -32,7 +32,7 @@ pub const SliceIterator = struct {
     }
 };
 
-test "clap.args.SliceIterator" {
+test "SliceIterator" {
     const args = &[_][]const u8{ "A", "BB", "CCC" };
     var iter = SliceIterator{ .args = args };
 
diff --git a/clap/comptime.zig b/clap/comptime.zig
index e8aae30..1570eaf 100644
--- a/clap/comptime.zig
+++ b/clap/comptime.zig
@@ -146,7 +146,7 @@ pub fn ComptimeClap(
     };
 }
 
-test "clap.comptime.ComptimeClap" {
+test "" {
     const Clap = ComptimeClap(clap.Help, clap.args.SliceIterator, comptime &[_]clap.Param(clap.Help){
         clap.parseParam("-a, --aa    ") catch unreachable,
         clap.parseParam("-b, --bb    ") catch unreachable,
diff --git a/clap/streaming.zig b/clap/streaming.zig
index af57e8f..602d94a 100644
--- a/clap/streaming.zig
+++ b/clap/streaming.zig
@@ -3,10 +3,12 @@ const clap = @import("../clap.zig");
 const std = @import("std");
 
 const args = clap.args;
-const testing = std.testing;
+const debug = std.debug;
 const heap = std.heap;
+const io = std.io;
 const mem = std.mem;
 const os = std.os;
+const testing = std.testing;
 
 /// The result returned from ::StreamingClap.next
 pub fn Arg(comptime Id: type) type {
@@ -76,7 +78,7 @@ pub fn StreamingClap(comptime Id: type, comptime ArgIterator: type) type {
                                     continue;
                                 if (param.takes_value == .None) {
                                     if (maybe_value != null)
-                                        return err(diag, param.names, error.DoesntTakeValue);
+                                        return err(diag, arg, .{ .long = name }, error.DoesntTakeValue);
 
                                     return Arg(Id){ .param = param };
                                 }
@@ -86,11 +88,13 @@ pub fn StreamingClap(comptime Id: type, comptime ArgIterator: type) type {
                                         break :blk v;
 
                                     break :blk (try parser.iter.next()) orelse
-                                        return err(diag, param.names, error.MissingValue);
+                                        return err(diag, arg, .{ .long = name }, error.MissingValue);
                                 };
 
                                 return Arg(Id){ .param = param, .value = value };
                             }
+
+                            return err(diag, arg, .{ .long = name }, error.InvalidArgument);
                         },
                         .short => return try parser.chainging(.{
                             .arg = full_arg,
@@ -105,10 +109,12 @@ pub fn StreamingClap(comptime Id: type, comptime ArgIterator: type) type {
 
                                 return Arg(Id){ .param = param, .value = arg };
                             }
+
+                            return err(diag, arg, .{}, error.InvalidArgument);
                         },
                     }
 
-                    return err(diag, .{ .long = arg }, error.InvalidArgument);
+                    unreachable;
                 },
                 .chaining => |state| return try parser.chainging(state, diag),
             }
@@ -138,28 +144,32 @@ pub fn StreamingClap(comptime Id: type, comptime ArgIterator: type) type {
                     }
                 }
 
-                if (param.takes_value == .None)
+                const next_is_eql = if (next_index < arg.len) arg[next_index] == '=' else false;
+                if (param.takes_value == .None) {
+                    if (next_is_eql)
+                        return err(diag, arg, .{ .short = short }, error.DoesntTakeValue);
                     return Arg(Id){ .param = param };
+                }
 
                 if (arg.len <= next_index) {
                     const value = (try parser.iter.next()) orelse
-                        return err(diag, param.names, error.MissingValue);
+                        return err(diag, arg, .{ .short = short }, error.MissingValue);
 
                     return Arg(Id){ .param = param, .value = value };
                 }
 
-                if (arg[next_index] == '=')
+                if (next_is_eql)
                     return Arg(Id){ .param = param, .value = arg[next_index + 1 ..] };
 
                 return Arg(Id){ .param = param, .value = arg[next_index..] };
             }
 
-            return err(diag, .{ .short = arg[index] }, error.InvalidArgument);
+            return err(diag, arg, .{ .short = arg[index] }, error.InvalidArgument);
         }
 
-        fn err(diag: ?*clap.Diagnostic, names: clap.Names, _err: anytype) @TypeOf(_err) {
+        fn err(diag: ?*clap.Diagnostic, arg: []const u8, names: clap.Names, _err: anytype) @TypeOf(_err) {
             if (diag) |d|
-                d.name = names;
+                d.* = .{ .arg = arg, .name = names };
             return _err;
         }
     };
@@ -187,7 +197,33 @@ fn testNoErr(params: []const clap.Param(u8), args_strings: []const []const u8, r
         unreachable;
 }
 
-test "clap.streaming.StreamingClap: short params" {
+fn testErr(params: []const clap.Param(u8), args_strings: []const []const u8, expected: []const u8) void {
+    var diag: clap.Diagnostic = undefined;
+    var iter = args.SliceIterator{ .args = args_strings };
+    var c = StreamingClap(u8, args.SliceIterator){
+        .params = params,
+        .iter = &iter,
+    };
+    while (c.next(&diag) catch |err| {
+        var buf: [1024]u8 = undefined;
+        var slice_stream = io.fixedBufferStream(&buf);
+        diag.report(slice_stream.outStream(), err) catch unreachable;
+
+        const actual = slice_stream.getWritten();
+        if (!mem.eql(u8, actual, expected)) {
+            debug.warn("\n============ Expected ============\n", .{});
+            debug.warn("{}", .{expected});
+            debug.warn("============= Actual =============\n", .{});
+            debug.warn("{}", .{actual});
+            testing.expect(false);
+        }
+        return;
+    }) |_| {}
+
+    testing.expect(false);
+}
+
+test "short params" {
     const params = [_]clap.Param(u8){
         clap.Param(u8){
             .id = 0,
@@ -239,7 +275,7 @@ test "clap.streaming.StreamingClap: short params" {
     );
 }
 
-test "clap.streaming.StreamingClap: long params" {
+test "long params" {
     const params = [_]clap.Param(u8){
         clap.Param(u8){
             .id = 0,
@@ -283,7 +319,7 @@ test "clap.streaming.StreamingClap: long params" {
     );
 }
 
-test "clap.streaming.StreamingClap: positional params" {
+test "positional params" {
     const params = [_]clap.Param(u8){clap.Param(u8){
         .id = 0,
         .takes_value = .One,
@@ -299,7 +335,7 @@ test "clap.streaming.StreamingClap: positional params" {
     );
 }
 
-test "clap.streaming.StreamingClap: all params" {
+test "all params" {
     const params = [_]clap.Param(u8){
         clap.Param(u8){
             .id = 0,
@@ -366,3 +402,31 @@ test "clap.streaming.StreamingClap: all params" {
         },
     );
 }
+
+test "errors" {
+    const params = [_]clap.Param(u8){
+        clap.Param(u8){
+            .id = 0,
+            .names = clap.Names{
+                .short = 'a',
+                .long = "aa",
+            },
+        },
+        clap.Param(u8){
+            .id = 1,
+            .names = clap.Names{
+                .short = 'c',
+                .long = "cc",
+            },
+            .takes_value = .One,
+        },
+    };
+    testErr(&params, &[_][]const u8{"q"}, "Invalid argument 'q'\n");
+    testErr(&params, &[_][]const u8{"-q"}, "Invalid argument '-q'\n");
+    testErr(&params, &[_][]const u8{"--q"}, "Invalid argument '--q'\n");
+    testErr(&params, &[_][]const u8{"--q=1"}, "Invalid argument '--q'\n");
+    testErr(&params, &[_][]const u8{"-a=1"}, "The argument '-a' does not take a value\n");
+    testErr(&params, &[_][]const u8{"--aa=1"}, "The argument '--aa' does not take a value\n");
+    testErr(&params, &[_][]const u8{"-c"}, "The argument '-c' requires a value but none was supplied\n");
+    testErr(&params, &[_][]const u8{"--cc"}, "The argument '--cc' requires a value but none was supplied\n");
+}