AVM: Add Semicolon Parsing#4363
Conversation
jannotti
left a comment
jannotti
left a comment
There was a problem hiding this comment.
This looks almost ready. Thanks for all those test changes, I'm sure it was a pain.
Simplify things by lexing ";" in fieldsFromLine
Codecov Report
@@ Coverage Diff @@
## master #4363 +/- ##
=======================================
Coverage 55.60% 55.61%
=======================================
Files 403 403
Lines 50801 50811 +10
=======================================
+ Hits 28250 28260 +10
Misses 20157 20157
Partials 2394 2394
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
more concise token tests
| } | ||
| } | ||
| return tokens, nil | ||
| } |
There was a problem hiding this comment.
Is the fact that splitTokens() behaves differently for the following 2 examples problematic? (the 2nd output isn't consistently nil):
tokens := []string{"hello", "there"}
splitTokens(tokens)
tokens = []string{"hello", "there", ";"}
splitTokens(tokens)See the playground
There was a problem hiding this comment.
I believe the difference will turn out to be important in the next step of the plan. Once tokens can expand into other tokens, it will matter whether something expands to included the semi or not. So I think the distinction is useful. (But in the meantime, the distinction won't matter because the next tokens are tested with len.
There was a problem hiding this comment.
This seems like a reasonable change. I made a suggestion regarding adding more test cases to TestFieldsFromLine. You might also consider adding a unit test for the new helpersplitTokens(). Though it is implicitly tested in TestFieldsFromLine adding more unit tests to such an important part of the codebase seems beneficial to me.
|
Thanks for the feedback Zeph! I've gone through and addressed all the comments I believe |
| check(`"" // test`, `""`) | ||
| check("int 1; int 2", "int", "1", ";", "int", "2") | ||
| check("int 1;;;int 2", "int", "1", ";", ";", ";", "int", "2") | ||
| check("int 1; ;int 2;; ; ;; ", "int", "1", ";", ";", "int", "2", ";", ";", ";", ";", ";") |
There was a problem hiding this comment.
How about:
"int 1; ;int 2;; ; ;;"
*"int 1; ;int 2;"";""; ; ;;;;"" ;"" ; "
There was a problem hiding this comment.
(or slight variants that are sensible given how the test works)
There was a problem hiding this comment.
good point, I added most of them (the first I think was pretty covered already so I left that out)
3 participants
Summary
Test Plan