Hejsil / mecha

A parser combinator library for Zig
MIT License
453 stars 20 forks source link

most mecha.ascii.* parsers are Parser(u8), but mecha.ascii.char is Parser(void) #42

Closed lotheac closed 1 year ago

lotheac commented 1 year ago

Hi, thank you for this library.

The following test fails:

const testing = @import("std").testing;
const mecha = @import("mecha");

const hash = mecha.ascii.char('#');
const digit = mecha.ascii.digit(10);
const either = mecha.oneOf(.{ hash, digit });

test {
    try mecha.expectResult(u8, .{ .value = '#' }, hash(testing.allocator, "#"));
    try mecha.expectResult(u8, .{ .value = '5' }, digit(testing.allocator, "5"));
    try mecha.expectResult(u8, .{ .value = '9' }, either(testing.allocator, "9"));
    try mecha.expectResult(u8, .{ .value = '#' }, either(testing.allocator, "#"));
}
foo.zig:9:55: error: expected type 'error{OtherError,OutOfMemory,ParserFailed}!mecha.Result(u8)', found 'error{OtherError,OutOfMemory,ParserFailed}!mecha.Result(void)'
    try mecha.expectResult(u8, .{ .value = '#' }, hash(testing.allocator, "#"));
                                                  ~~~~^~~~~~~~~~~~~~~~~~~~~~~~
foo.zig:9:55: note: error union payload 'mecha.Result(void)' cannot cast into error union payload 'mecha.Result(u8)'
mecha.zig:23:12: note: struct declared here (2 times)
    return struct {
           ^~~~~~

This happens because mecha.ascii.char's result type is void, in contrast to the other pub fn parsers in this file (and it seems the same is true for mecha.utf8). I found that difference quite surprising, since it leads to somewhat unintuitive code, eg. when using oneOf() on ascii.range/ascii.digit and ascii.char. Could you shed some light on why the char parsers use discard?

Hejsil commented 1 year ago

I think the reasoning was that since you are looking for exactly one char, the returned value would always just be that char, so the information is redundant. I think in isolation that reasoning makes sense, but as you have showed, it makes the function play less well with the rest of the library, so it should probably be changed