Allow multiple start states for a rule. Support nested Java exceptions. #52
Conversation
|
|
||
| def self.rule(from_state, pattern, to_state) | ||
| Struct::Rule.new(from_state, pattern, to_state) | ||
| def self.rule(from_states, pattern, to_state) |
There was a problem hiding this comment.
thoughts on the interface of this function:
can we always assume from_states is Array, and clients always need to pass Array in? 2 benefits:
- no special logic to convert single element to an array
- the client side code are consistent.
There was a problem hiding this comment.
Instead of thinking of this as a function, think of this as a DSL for defining rules. We want to be able to define rules with a single transition, or with multiple transitions that share a regexp and the target state. In the former case we specify a single from state, and in the latter we specify an array of from states. Most rules will represent a single transition. The goal is to make the usage simpler, not the function implementation.
There was a problem hiding this comment.
[Optional]
- still think
from_statesimplies input of this parameter is a collection of items. - still think always pass
from_statesparameter as array makes the client side code consistent - so user of this function (or DSL) don't need to reason two modes to use thisrulefunction.
This comment falls under readability realm, so it's optional to change.
There was a problem hiding this comment.
Good point on the variable name — renamed to from_state_or_states.
The idea was to keep most of the existing rule statements unchanged.
| Metrics/ClassLength: | ||
| Max: 1300 | ||
|
|
||
| # Offense count: 1 |
There was a problem hiding this comment.
what's the purpose of this configuration change on rubocop?
There was a problem hiding this comment.
The module in exception_detector.rb became too large...
| RULES_BY_LANG.keys | ||
| end | ||
|
|
||
| JAVA_RULES = [ |
There was a problem hiding this comment.
[Optional] Yesterday we chat about how we have more strict regex at client side than server side. Have we considered something looser? Like:
JAVA_RULES = [
rule(:start_state,
/(?:Exception|Error|Throwable|V8 errors stack trace)[:\r\n]/,
:java),
rule(:java, /^[\t ]*nested exception is:[\t ]*$/, :java),
rule(:java, /^$/, :java),
rule(:java, /^[\t ]+(?:eval )?at /, :java),
rule(:java, /^[\t ]*(?:Caused by|Suppressed):/, :java),
rule(:java, /^[\t ]*... \d+\ more/, :java)
].freeze
There was a problem hiding this comment.
That would miss parsing the nested exception itself after the nested exception is: line. That's why I've introduced :java_start_exception. I suppose we could simply add a transition from :java to :java_start_exception on nested exception is:, but that would not make the rules much simpler. WDYT?
| :java_after_exception), | ||
| rule([:java_after_exception, :java], /^[\t ]*... \d+\ more/, :java) | ||
| rule([:java_after_exception, :java], | ||
| /^[\t ]*... \d+ (?:more|common frames omitted)$/, :java) |
There was a problem hiding this comment.
why adding $ suddenly? and I cannot verify whether java would not add '\n', or other characters after those text. Do you have reference on source code?
There was a problem hiding this comment.
Good point. The text ("more" or "common frames omitted") does appear at the end of the line, but log messages may also have linefeed/newline characters appended, so it's better to be careful. Fixed.
Fixes #29.