search

dlang-community / Pegged

A Parsing Expression Grammar (PEG) module, using the D programming language.
533 stars 66 forks source link

Grammartester consice branch rule #338

Closed ricardaxel closed 1 year ago

ricardaxel commented 1 year ago

Related #337

veelo commented 1 year ago

I like where this is going, good job.

The C example, though, seems to have been in a bad shape from the beginning. Unless I am terribly mistaken, cparser.d is the parser generated by Pegged. It should never be edited by a human, so your unit test should not be in there. The build system should automatically generate the parser, and CI should verify that it actually works as intended.

(I have been working on improving the other examples, but haven't pushed yet.)

I am confused about testerparser.d as well. I cannot find the grammar file in the repository, and it isn't in your diff either. I suspect it was never committed, and you recovered the grammar from the comment? Same here, the grammar should be committed, and the tester generated and tested.

It may make sense to split PRs so that new stuff is separated from fixes.

You may tell that I am not the original developer, I am the current maintainer :-)

I'll see if I can finalize my work on the other examples today, that checks whether examples actually work. It may make sense to also use the grammar tester on them in a later PR.

ricardaxel commented 1 year ago

The C example, though, seems to have been in a bad shape from the beginning. Unless I am terribly mistaken, cparser.d is the parser generated by Pegged. It should never be edited by a human, so your unit test should not be in there. The build system should automatically generate the parser, and CI should verify that it actually works as intended.

Ok I see, what I can do is move unittest in 'c.d' file and remove 'cparser.d'.

I am confused about testerparser.d as well. I cannot find the grammar file in the repository, and it isn't in your diff either. I suspect it was never committed, and you recovered the grammar from the comment? Same here, the grammar should be committed, and the tester generated and tested.

Yes you're right, the only tracks of the grammar is in the comment at the beginning of testerparser.d. For me this make sense to directly commit the generated code here to limit compilation time ? But I agree that grammar should be commited as well, and that tests shall ensure that it is always aligned with generated code.

veelo commented 1 year ago

Thanks!