chore: Keep hidden field to std.objectRemoveKey and support inheritan… #586
Conversation
He-Pin
force-pushed
the
objectRemoveKey
branch
from
8e7fda9 to
a11d6c9
Compare
He-Pin
force-pushed
the
objectRemoveKey
branch
from
a11d6c9 to
71046aa
Compare
stephenamar-db
left a comment
stephenamar-db
left a comment
There was a problem hiding this comment.
This PR needs another look. It's not ready.
Make sure to use Scala collections and operators when needed
| for (s <- objs.reverse) { | ||
| current = new Val.Obj(s.pos, s.getValue0, false, s.triggerAsserts, current) | ||
| // Preserve excludedKeys if they exist in the chain's keys OR in the collected excludedKeys | ||
| val filteredExcludedKeys = if (s.excludedKeys != null) { |
There was a problem hiding this comment.
use scala sets?
Also just use intersect?
https://docs.scala-lang.org/overviews/collections/sets.html
There was a problem hiding this comment.
The default Scala Set is immutable, not easy to use here, the current value cache is a java linkedHashMap, and IIRC, Java's HashMap is faster than Scala's
He-Pin
force-pushed
the
objectRemoveKey
branch
2 times, most recently
from
101839e to
ee0b34c
Compare
He-Pin
force-pushed
the
objectRemoveKey
branch
5 times, most recently
from
ef9709d to
2702fb2
Compare
He-Pin
force-pushed
the
objectRemoveKey
branch
from
2702fb2 to
fb4cb90
Compare
There was a problem hiding this comment.
Pull request overview
Ports upstream Jsonnet behavior for std.objectRemoveKey so it preserves hidden fields and interacts correctly with super/inheritance (including assertion behavior), alongside new regression tests.
Changes:
- Re-implement
std.objectRemoveKeyby delegating toVal.Obj.removeKeys(excluded-keys wrapper approach) rather than rebuilding a concrete object. - Extend object key gathering / lookup logic in
Val.Objto account for excluded keys across inheritance chains. - Add/expand stdlib + go test-suite coverage for hidden fields, inheritance, and
super/assert interactions.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sjsonnet/test/src-jvm-native/sjsonnet/BaseFileTests.scala | Improves debugging output when an error was expected but evaluation succeeded. |
| sjsonnet/test/resources/test_suite/stdlib.jsonnet | Adds stdlib-level assertions covering hidden fields + inheritance semantics for objectRemoveKey. |
| sjsonnet/test/resources/go_test_suite/builtinObjectRemoveKey_hidden.jsonnet | New regression test for hidden-field retention through objectRemoveKey. |
| sjsonnet/test/resources/go_test_suite/builtinObjectRemoveKey_hidden.jsonnet.golden | Golden output for hidden-field regression test. |
| sjsonnet/test/resources/go_test_suite/builtinObjectRemoveKey_super.jsonnet | New regression test for super behavior through objectRemoveKey. |
| sjsonnet/test/resources/go_test_suite/builtinObjectRemoveKey_super.jsonnet.golden | Golden output for super regression test. |
| sjsonnet/test/resources/go_test_suite/builtinObjectRemoveKey_super_assert.jsonnet | New regression test ensuring self/super asserts behave correctly with removed keys. |
| sjsonnet/test/resources/go_test_suite/builtinObjectRemoveKey_super_assert.jsonnet.golden | Golden output for assert regression test. |
| sjsonnet/src/sjsonnet/stdlib/ObjectModule.scala | Switches objectRemoveKey builtin implementation to Val.Obj.removeKeys. |
| sjsonnet/src/sjsonnet/Val.scala | Adds excludedKeys plumbing, removeKeys, and adjusts key gathering / field lookup accordingly. |
| sjsonnet/src/sjsonnet/Util.scala | Adds shared empty LinkedHashMap helper + small set utilities used by the new logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
refs: com-lihaoyi/mill#6814 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
He-Pin
force-pushed
the
objectRemoveKey
branch
from
fb4cb90 to
fe5801d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
port google/jsonnet#1269
refs: google/go-jsonnet#837
refs: google/go-jsonnet#830