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

Added grammar builder #128

Open
wants to merge 1 commit into
base: language-def-redesign
Choose a base branch
from

Conversation

hosford42
Copy link

@hosford42 hosford42 commented Sep 13, 2020

Code review checklist

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated
  • Docstrings have been included and/or updated, as appropriate
  • Standalone docs have been updated accordingly
  • Changelog(s) has/have been updated, as needed (see CHANGELOG.md, no need
    to update for typo fixes and such).

@hosford42
Copy link
Author

Hi, @igordejanovic! Let me know if this needs anything to get it up to par.

@igordejanovic
Copy link
Owner

@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.

Copy link
Owner

@igordejanovic igordejanovic left a 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.
Copy link
Owner

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

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

Copy link
Author

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

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 can add flake8 line length setting in config.cfg like this and check your source for violations.

@hosford42
Copy link
Author

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.

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