Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grammartester consice branch rule #338

Merged

Conversation

ricardaxel
Copy link

Related #337

@veelo
Copy link
Collaborator

veelo commented Sep 7, 2023

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
Copy link
Author

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.

@ricardaxel ricardaxel force-pushed the grammartester-consice-branch-rule branch from 1783a79 to 8db67ed Compare September 7, 2023 09:30
@veelo veelo merged commit 67fcd03 into dlang-community:master Sep 7, 2023
39 checks passed
@veelo
Copy link
Collaborator

veelo commented Sep 7, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants