Skip to content
This repository was archived by the owner on Sep 24, 2019. It is now read-only.

Issue3 rebase - #58

Merged
abargnesi merged 68 commits into
OpenBEL:masterfrom
nbargnesi:issue3-rebase
May 16, 2016
Merged

Issue3 rebase#58
abargnesi merged 68 commits into
OpenBEL:masterfrom
nbargnesi:issue3-rebase

Conversation

@nbargnesi

@nbargnesi nbargnesi commented May 16, 2016

Copy link
Copy Markdown
Member

PR to close #3 and #4. Will need to expand observed, simple, and nested statement incomplete specs further but I'm hoping we can merge this first considering the broad changes.

Will have to duplicate v1_syntax specs to v2_syntax_specs and make the appropriate modifications as well.

Nick and others added 30 commits May 16, 2016 10:49
Need to review the term_argument action with Tony.
Added incomplete instance variable to each rl.
- simplify - call out ID_CHARS and break up remaining machine
  definitions (IDENT)
- relocate yield action to instantiation
Need to go back and prune the old rules out.
I mixed actions with definitions. Oops. Also @'d when I should have %'d,
and that's always a problem. Don't do that.
Still have problems parsing unclosed lists.
This is currently problematic since there are yields inside definitions
where there shouldn't be. This is causing list specs to succeed and set
specs to fail.
@abargnesi

Copy link
Copy Markdown
Member

Nice work! Extensive changes to support complete/incomplete AST results.

Minor points:

  • Some parsers (e.g. comment_line, statement) result in Ruby stacktraces. Easy fixes to follow up with and test post merge.
  • More instance variables were created versus using @buffers hash. Was there a particular design choice here?
  • The standard error prints are useful to have in a debugging session. It would be nice to factor out debug logging into a separate Ruby module that is enabled via an environment variable.
  • Tests seem extensive so we should review the old spec tests and remove as appropriate.

These point shouldn't stop us from merging. I can follow up with some issues once I start to use them.

Thanks!

@abargnesi
abargnesi merged commit b5723cc into OpenBEL:master May 16, 2016
abargnesi pushed a commit that referenced this pull request May 17, 2016
This moved at some point.

All but one test passes.

refs #58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow incomplete AST results.

2 participants