Detect duplicate fields in the parser #86
Conversation
| @@ -303,5 +303,8 @@ object ErrorTests extends TestSuite{ | |||
| // | at .(sjsonnet/test/resources/test_suite/error.invariant.simple3.jsonnet:18:10) | |||
| // |""".stripMargin | |||
| // ) | |||
| test("duplicate_fields") - check( | |||
There was a problem hiding this comment.
Could we put this in EvaluatorTests instead? Currently all the test cases in test_suite/ that are used in FileTests.scala and ErrorTests.scala are directly vendored from the google/jsonnet implementation, so it's best to keep our own custom additions separate so next time we update the vendored code we don't accidentally lose our additions
There was a problem hiding this comment.
EvaluatorTests seems like a strange place for a parser test.
I also assumed that the test suite should be the original Jsonnet one, but then I saw that it had a few changes applied over time and it doesn't match the current Jsonnet test suite.
There was a problem hiding this comment.
How about ParserTests? Not much in there now, but if this is a parser test then it seems like it belongs
|
One small comment, but feel free to merge once resolved |
- The error message has a rather unhelpful position but this is consistent with the way other static errors are currently handled by Sjsonnet. The original Jsonnet parser does it better. - There are two different places in the Jsonnet codebase where duplicate fields are detected: Statically in the parser and dynamically in the VM. It's not clear to me without digging much deeper into their C++ implementation when the second one is supposed to kick in. There is no specification or test case for the dynamic detection. Fixes databricks#69
|
Test moved to ParserTests. |
The error message has a rather unhelpful position but this is consistent with the way other static errors are currently handled by Sjsonnet. The original Jsonnet parser does it better.
There are two different places in the Jsonnet codebase where duplicate fields are detected: Statically in the parser and dynamically in the VM. It's not clear to me without digging much deeper into their C++ implementation when the second one is supposed to kick in. There is no specification or test case for the dynamic detection.
Fixes #69