feat: add string object value#61
Conversation
summer-ji-eng
left a comment
summer-ji-eng
left a comment
There was a problem hiding this comment.
Should we add a unit test under /test/ast
|
The unit test is added in |
miraleung
left a comment
miraleung
left a comment
There was a problem hiding this comment.
@summer-ji-eng Since StringObjectValue just holds objects, we're fine without a unit test. However, we should have unit tests for classes that have additional logic (e.g. type-checking). The writer visitor test is not intended as a replacement, and checks only for the serialization logic.
LGTM assuming the nit will be addressed.
| /** =============================== EXPRESSIONS =============================== */ | ||
| @Test | ||
| public void writeStringObjectValue() { | ||
| StringObjectValue s = StringObjectValue.builder().setValue("\"test\"").build(); |
There was a problem hiding this comment.
Should have picked this up in the last review, but here we want to test that the writer visitor actually writes this string. The test here would go into ast/StringObjectValueTest.java. In the general case, the caller wouldn't pass in "\"test\"" unless they wanted to print \"test\".
| } | ||
|
|
||
| public static StringObjectValue withValue(String value) { | ||
| return builder().setValue(value).build(); |
There was a problem hiding this comment.
The test case reminded me that we should also handle special and escaped characters, e.g. if someone passes a quote in here, or escaped characters.
| VariableExpr variableExpr = | ||
| VariableExpr.builder().setVariable(variable).setIsDecl(true).build(); | ||
|
|
||
| Value value = StringObjectValue.withValue("\"test\""); |
There was a problem hiding this comment.
In the expected behavior below, the caller should be doing withValue("test") IMO.
|
This needs fixing, let's not merge this yet. |
On second thought, we do need a unit test for handling special characters. |
miraleung
left a comment
miraleung
left a comment
There was a problem hiding this comment.
Shouldn't there be a corresponding change for the writer visitor?
|
what does |
* updated CHANGELOG.md [ci skip] * updated README.md [ci skip] * updated versions.txt [ci skip] * updated pom.xml Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
3 participants
No description provided.