- Notifications
You must be signed in to change notification settings - Fork32
Added grammar builder#128
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:language-def-redesign
Are you sure you want to change the base?
Added grammar builder#128
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Hi,@igordejanovic! Let me know if this needs anything to get it up to par. |
@hosford42 Thanks much for the contribution! I just skimmed through the implementation and it looks very good. I'll do the full review in the next couple of days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@hosford42 Excellent work! Everything looks good. I have just some minor styling issues (please see bellow). It would be nice to add some docs for this feature but I think that can wait until this gets merged to master.
| This method only gets called if the normal lookup mechanism for an attribute fails. In other words, we only | ||
| get here if someone requests an attribute that this object doesn't have. When this happens, we call self.end() | ||
| and then get try to get the attribute from the parent. If we are correct in doing so, everything is beautiful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
typo: get try to get
| @@ -0,0 +1 @@ | |||
| __version__ = None | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This file is autogenerated by setuptools_scm so should not be committed to git repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So sorry, this was not supposed to have been included in the commit! I only fudged it so I could work directly off the development version while debugging.
| from typing import Dict, Union, List, Iterator, TypeVar, ContextManager, Optional, Callable, Any, Sequence | ||
| from parglare.exceptions import GrammarBuilderValidationError | ||
| from parglare.grammar import MULT_ONE_OR_MORE, MULT_ZERO_OR_MORE, MULT_OPTIONAL, Terminal, MULT_ONE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
parglare is using PEP-8. That means 79 line length. I'm ok to go up to 90. You canadd flake8 line length setting in config.cfg like this and check your source for violations.
Hi@igordejanovic. I'll try to get these changes done later today. Thanks for the feedback, and I'm happy to hear you are pleased. |
Uh oh!
There was an error while loading.Please reload this page.
Code review checklist
CHANGELOG.md, no needto update for typo fixes and such).