teragrep / pth_03

Data Processing Language (DPL) parser
GNU Affero General Public License v3.0
0 stars 5 forks source link

Fix head command and match the whole query to the grammar #52

Closed 51-code closed 2 months ago

51-code commented 3 months ago

Fixes issues #8 and #39.

This PR introduces two changes:

So firstly the head command:

Explanation on the grammar requiring the whole query to be written correctly: I have added an EOF (end-of-file) token in the end of the root grammar rule. This means that now the whole query has to be grammatically correct, not just a subset like before. This oversight introduced many bugs where the parser doesn't throw any exceptions in certain cases that are explained in #39. A query would only parse until some point, leaving the rest of the query, perhaps whole commands, ineffective from the user's point of view.

This change made a whole bunch of tests fail, as many queries in the tests already had the bug. I have written these queries correctly, or if they queries were correct, I disabled the tests and made an issue about the grammar problems the respective commands have.

51-code commented 2 months ago

Noticed that it wasn't possible to use just the head command without any parameters after my changes. Changed this back to how it was meant to be and made a test for that as well.

kortemik commented 2 months ago

is there a metaissue for re-enabling the broken tests that were disabled in this commit? why there are changes to test payload files? if tests are broken, they should be disabled, if input is incorrect, it should be mentioned that they were not correct and be corrected.

51-code commented 2 months ago

There has been issues opened about all the disabled tests individually, would it be okay if I edited those issues to inform which tests should be enabled after solving them? Or just a new issue for all of the disabled issues together?

For the latter question: I went through the broken tests (the tests that were throwing an exception with the root rule change). Some of the tests were simply broken because of incorrect test payloads and I fixed those. If the payload was correct, I disabled the test (this would mean that the grammar itself is broken). Should I mention theses incorrect payloads in the code, or where? A general note about that was in the bottom of the PR message.

kortemik commented 2 months ago

Please add the note about incorrect payload and link to pr into the individually created issues. Please create a meta-issue and link these issues into the meta-issue.

51-code commented 2 months ago

Please add the note about incorrect payload and link to pr into the individually created issues. Please create a meta-issue and link these issues into the meta-issue.

Disabled tests have now been listen in each issue separately. A note has been added for test payload changes as well. Meta issue about re-enabling the issues has been created and the issues in question has been listed in it.

Furthermore, I changed two payloads back to how they were: tickets71_A and tickets_71. These were in fact grammatically correct. I created a new test for these and disabled that as it still fails with the current grammar.

I have not made notes about the changes in payloads delta.txt and chart4.txt. These do not have any open issues because the payloads were grammatically incorrect, which lead to their corresponding tests failing. So, the grammar is fine and has no issues, but the test payload was wrong, giving a false negative.

kortemik commented 2 months ago

approved!