shnewto / bnf

Parse BNF grammar definitions
MIT License
256 stars 22 forks source link

Parse could return Result #12

Closed CrockAgile closed 6 years ago

CrockAgile commented 6 years ago

Parsing a grammar can fail. Currently, if parsing fails an empty grammar is returned and errors are printed to stdout. Users would likely be interested in detecting if the parsing failed, but capturing stdout or stderr is not ideal. The code required to currently detect an empty grammar would look something like:

let grammar = bnf::parse(input);
// errors printed to stdout
if grammar.productions.is_empty() {
    println!("empty grammar");
} else {
    println!("non-empty grammar");
}

Instead of printing error strings to stdout or stderr, they could be included in the Result<Grammar,Error> type, allowing for more traditional error handling:

let grammarResult = bnf::parse(input);

match grammarResult {
    Ok(grammar) => println!("working grammar: {}", grammar),
    Err(e) => eprintln!("grammar parsing failed: {}", e),
}

If this change were to be made, would reporters still be involved? As a library it seems strange to output to stdout or stderr instead of output being handled in the code. But as a command line tool, error reporting in terminal is definitely valuable. Maybe BNF should include an optional CLI binary intended for terminal use? I forget if cargo allows for dual binary & library crates.

shnewto commented 6 years ago

Good point, I agree that relying on stdout or stderr isn't totally reasonable. Probably providing the error report in a bnf::parse Result would be the cleaner and more intuitive functionality for anyone incorporating the library. Thanks for raising the issue, this one seems like it should have a reasonably high priority (I'm slightly concerned with introducing a breaking change just 3 days after first publishing the crate though haha :cold_sweat:)

CrockAgile commented 6 years ago

Closed by PR #24