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

Refactor: Parser Improvements, Generics, and helpers #19

Merged
merged 5 commits into from
Nov 3, 2020
Merged

Conversation

lytefast
Copy link
Contributor

@lytefast lytefast commented Oct 24, 2020

  • add vararg variant to Parser.addRules for simpler addition of rules
  • update generics for adding rules
  • update control flow to be more explicit about what happens if there's no found rule
  • update ParserTest to kotlin and add failure test case for ^
  • Don't need the T: Node<R> generic on Parser
    • It didn't offer anything as T was always Node<R> since all the rules generated random nodes. We we also never used it besides just returning T
    • this allows us to drop a few more null checks as well since ParseSpec.root is now non-null

Now the flow looks something like this:

val (rule, matcher) = rules
    .firstMapOrNull { rule ->
      val matcher = rule.match(inspectionSource, lastCapture, builder.state)
      // log + return
    }
    ?: throw ParseException("failed to find rule to match source", source)
// Found logic

vs

var foundRule = false
for (rule in rules) {
  val matcher = rule.match(inspectionSource, lastCapture, builder.state)
  if (matcher != null) {
    logMatch(rule, inspectionSource)
    foundRule = true
    // Found logic
    break
  } else {
    logMiss(rule, inspectionSource)
  }
}

if (!foundRule) {
  throw ParseException("failed to find rule to match source", source)
}

@lytefast lytefast self-assigned this Oct 24, 2020
@lytefast lytefast requested a review from AndyG October 24, 2020 23:04
treeMatcher.registerDefaultMatchers()
}

@Test(expected = Parser.ParseException::class)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this test

It didn't offer anything as `T` was always `Node<R>` since all the rules generated random nodes.
We we also never used it besides  just returning T
@lytefast
Copy link
Contributor Author

lytefast commented Nov 3, 2020

Got the offline ✔️

@lytefast lytefast merged commit 4197e64 into master Nov 3, 2020
@lytefast lytefast deleted the opt/parser branch November 3, 2020 00:13
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.

1 participant