-
Notifications
You must be signed in to change notification settings - Fork 280
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
Add properties and methods to Mapping
/Listing
API to resemble Map
/List
more
#672
Conversation
/// The values contained in this mapping. | ||
external values: List<Value> | ||
|
||
/// The entries contained in this mapping. | ||
external entries: List<Pair<Key, Value>> |
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.
Is there a particular reason why there are List
s while keys
is a Set
? Mappings are ordered (and documented as such), so this sort of makes sense to me. Having this reasoning documented might be helpful.
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.
IMHO these should be Listing
s, and keys
should be changed to Listing
via a deprecation cycle.
While we are here, what is the reason that pkl has Mappings and Map and Listing and List? 🤔 |
Several reasons (not comprehensive):
It'd be good to simplify the language and not have this dichotomy, but we'd need to at least solve the above challenges first. |
var keys = getAllKeys(); | ||
var builder = new VmObjectBuilder(keys.getLength()); | ||
for (var key: keys) { | ||
var member = VmUtils.readMember(this, key); |
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.
Instead of forcing and copying each value, the Dynamic could delegate to the Mapping via addEntry(Object key, SharedMemberNode valueNode)
.
A similar overload could be added for addProperty
(I didn't need it at the time).
Closing this as an outdated duplicate of #683. |
No description provided.