Refactor load_grammar to use Terminal/NonTerminal throughout the process #1018
Conversation
|
(I didn't move anything to a new module yet, so the diff will be easy to read) |
| template_source=(name if params else None)) | ||
|
|
||
|
|
||
| class Definition: |
There was a problem hiding this comment.
we are Python3.6+, this can be a dataclass.
There was a problem hiding this comment.
I wish, but it isn't built-in for 3.6, you need to pip install dataclasses
We could add a requirement, but I'm not sure if it's worth it.
There was a problem hiding this comment.
Right, I always forget that :-/ And considering that we have about 10,000 daily downloads on that version, I wouldn't add that.
There was a problem hiding this comment.
On the other hand, Python3.6 reaches end of life soon.
There was a problem hiding this comment.
Soon enough we'll only support Python 5 and up.
Anyway, I'm all for using dataclasses as soon as it makes sense. I'm just not sure if it's now yet.
There was a problem hiding this comment.
Give them half a year to one year time after EOF, then get away from python3.6 . I think that will be a decent rhythm for all python releases.
Codecov Report
@@ Coverage Diff @@
## master #1018 +/- ##
==========================================
+ Coverage 87.93% 87.97% +0.04%
==========================================
Files 49 49
Lines 7051 7075 +24
==========================================
+ Hits 6200 6224 +24
Misses 851 851
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@MegaIng Looks like both the tests and I missed two glaring bugs in the refactor, so if you have the time to give it another look, it's probably a good idea. Maybe it's impossible to catch everything. But I do wonder if there's a way to do it better, so we won't have to find them by "luck". |
|
Unless we go throu the work and prove that everything is correct, we will never get there. But looking at the coverage report, there are a lot of things that can be change so that they are tested, for example completely unexpected characters in the grammar file, wrong template application. Also, you might have broken Also, maybe we can tell cov to ignore lines that start with |
|
Actually, those two bugs wouldn't be caught by the coverage report. I'll check %ignore, but I think we have it well covered in the tests? |
|
@MegaIng You were right about %ignore. I guess coverage is good for something. I also found another miss that was hidden for a long time. |
|
Did you try to run the python example for the entire stdlib? |
|
Yes, why? That's how I found those two previous bugs. |
|
Just making sure. Then I think this is done. |
|
Great. Would you like to take it from here, to add a way to parse any grammar format? I'm thinking something like an option, A nice test would be to give it |
|
Yeah, I can do that. Btw, do we want abnf as a default packaged grammar or as a plugin/example? |
|
I'm not sure. I'm a little worried about bloat, but maybe ABNF is standard enough to warrant being built-in. What do you think? |
|
The exact same thing xD. I think that because #1017 correctly also implements a lot of stuff that is not directly just the syntax, but also tests + examples + helper decorators, which would be quite a bit of extra bloat (in addition to the syntax itself which I would tolerate) we should reject it. Instead I am going to make a registry so that it is possible to use |
|
I don't see the harm in a few more tests and examples. But I agree additions to the actual library should be minimal. |
It took a bit of refactoring, but I think I got it done without too many changes.
Maybe I'm missing something, but apparently it's not necessary to change mangle, aliases, or the rest.