merged BNF and ABNF parsing

[Carlyle-Foster]

this change allowed me to compress the tests for BNF and ABNF syntax parsing.

this time the diff should be much clearer.

[coveralls]

coverage: 97.02% (-0.09%) from 97.109%
when pulling 14027a1 on Carlyle-Foster:main
into a8e52a3 on shnewto:main.

[Carlyle-Foster]

could i add the commit that changes parsers/mod.rs to parsers.rs to this? u could still see the good diff by looking at the changes only from the first commit and u could just not squash them so it ends up the same way as if i sent that PR later

[Carlyle-Foster]

@shnewto is there any progress on reviewing this?

[shnewto]

Hey @Carlyle-Foster, first off thanks for all your work! I'd really like you to continue your fantastic contributions, I think we just need to dial the process a bit before we get much further in.

re:

u could still see the good diff by looking at the changes only from the first commit

That's not really a flow I'm happy with, I'd like PR diffs and history to be understandable on their own. I'd also appreciate more context for your changes and the decisions you made in your PR description! As I mentioned in another PR, we're not in any rush and these changes are sophisticated enough that messy diffs and messy history make reviewing a lot more challenging.

Some recommendations:

• Don't continue to use the same branch after a PR is merged, always start fresh.
• If the commit history and diffs are really messy:
  • Manually copy the important files somewhere (manually as in, drag and drop or cp them somewhere outside the repo directory)
  • Start a fresh branch off of main with all its most recent changes, then manually copy your important files back to the codebase

I really do appreciate your work and engagement with this project! Thank you!

[Carlyle-Foster]

@shnewto the main reason i think we should merge BNF and ABNF parsing is simply that they're both compatible, and the complexity incurred by our current approach of trying to infer the Format is completely unecessary, and the inference itself is a possible point of failure + to top it all of, terminals don't even rightfully have a Format

i've reduced this PR down to the barest essentials now, i think it ready? the only thing that could rightfully go in parsers/bnf.rs and parsers/augmented.rs given the removal of Format is their tests, and since these are no longer duplicated between Formats i deleted one of them and renamed the other to parsers/match_tests.rs to reflect it's little remaining purpose, i'm planning to later merge it and parsers/nom_xt.rs into the main parsers module and thus make it a single file module again

[Carlyle-Foster]

That's not really a flow I'm happy with, I'd like PR diffs and history to be understandable on their own

it think their's a bit of a misunderstanding here, what i was suggesting would result in an identical git history to simply merging 2 PR's sequentially, or at least that was the intention

sometimes a conceptually atomic change that should be 1 PR can be most clearly expressed as multiple diffs/commits, such as in this case where the refactor implies a change in the directory structure but including the file shuffling in the same commit would obscure the diff

[CrockAgile]

@Carlyle-Foster what's the motivation behind merging the two parsers? because I am open to the idea, but my initial reaction is that allowing for mixing syntax is a bit messy, and potentially confusing to new users.

#[test]
fn should_not_allow_mixing_bnf_and_abnf_syntax() {
    let grammar = "
    <dna> = <base> / (base <dna>)
    base ::= 'A' / 'C' | 'G' / 'T' 
    ";
    let result: Result<Grammar, _> = grammar.parse();
    assert!(result.is_err(), "Grammar should not be valid");
}

Also, moving different syntaxes to dynamic time parsing, as opposed to static generics has a performance cost. But a small one! So it could be justified for a good reason

[Carlyle-Foster]

i elaborated a few posts above u

the main reason i think we should merge BNF and ABNF parsing is simply that they're both compatible, and the
complexity incurred by our current approach of trying to infer the Format is completely unecessary, and the inference
itself is a possible point of failure + to top it all of, terminals don't even rightfully have a Format

[CrockAgile]

Ah sorry missed that in the thread. I see the point about auto inferring being a point of failure. maybe the other end of the spectrum could be a solution? implementing parse for both formats parse<F> but defaulting to BNF ?

[Carlyle-Foster]

how is that different from what we were doing before, or do i misunderstand?

[Carlyle-Foster]

what's the current status here?