-
Notifications
You must be signed in to change notification settings - Fork 229
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
PAN: handle multiline tokens better during flattening #5292
Conversation
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.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sfraint)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto_nested/PaloAltoNestedFlattener.java, line 112 at r1 (raw file):
for (WordContext wordCtx : line) { sb.append(" "); int orgLine = wordCtx.WORD().getSymbol().getLine();
is there a strong reason to do this over wordCtx.getStart().getLine()
? If yes, should this be documented?
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.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @progwriter)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto_nested/PaloAltoNestedFlattener.java, line 112 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
is there a strong reason to do this over
wordCtx.getStart().getLine()
? If yes, should this be documented?
No, in this case they should be the same.
Codecov Report
@@ Coverage Diff @@
## master #5292 +/- ##
============================================
- Coverage 73.39% 73.38% -0.01%
+ Complexity 31956 31954 -2
============================================
Files 2624 2624
Lines 128893 128897 +4
Branches 15499 15500 +1
============================================
- Hits 94602 94596 -6
- Misses 26802 26810 +8
- Partials 7489 7491 +2
|
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.
Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
During flattening, some tokens may span multiple lines (e.g. quoted strings:
"something\nsomething"
). Currently, when the line map is being updated, these multiline tokens are not taken into account, so mapping for any line after a multiline token is shifted and incorrect.This PR takes into account multiline tokens when populating the line map. After this PR, all lines in the output text corresponding to the multiline token are mapped back to the original config line containing the token start.