-
Notifications
You must be signed in to change notification settings - Fork 62
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
Record and Tuple object model spec #132
Conversation
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.
Some initial comments
Thanks dan, I'll make another pass on this tomorrow |
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.
Some editorial comments on the Record side, but things seem to be getting in good shape; I mostly just have editorial comments. The Tuple side seems to be in progress so I'll leave that for a later review.
<h1>RecordCreate ( _value_ )</h1> | ||
<p>This abstract operation RecordCreate takes a _value_ argument. It is used to specify the creation of new Record exotic objects. It performs the following steps when called:</p> | ||
<emu-alg> | ||
1. Assert: Type(_value_) is a Record. |
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.
why would RecordCreate take a Record? This seems like more the domain of https://tc39.es/ecma262/#sec-toobject than of its own abstract op
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.
I was a little mystified by the naming convention at first, but see analogous operations like https://tc39.es/ecma262/#sec-stringcreate and https://tc39.es/ecma262/#sec-modulenamespacecreate
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.
hmm, strange that String has it but Number, Boolean, etc don't (objects having a FooCreate makes sense to me; but primitives don't). It feels like the precedent is more for primitives not to have one.
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.
I guess what distinguishes String is that its wrapper is exotic, like Record. Maybe we should continue this discussion in an editorial issue in ecma262? I want to make sure we have clear text somewhere or other to explain how the wrapper is set up, and I think it should match String, but I am fine with moving to some other way of organizing the text.
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.
Please open an issue if you think this should still be changed @ljharb , we can discuss the choices in more detail.
Thanks Dan, I will try to get around it soon! |
Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
…cord-tuple into rb/ops-and-algs
1. Assert: IsPropertyKey(_P_) is *true*. | ||
1. If Type(_P_) is a Symbol, return *undefined*. | ||
1. Let _getResult_ be ! RecordGet(_R_, _P_). | ||
1. If _getResult_ is ~empty~, return *undefined*. |
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.
does this mean i can't set an own expando string property on a boxed Record? var o = Object('a'); o.foo = true; o.foo;
works, for example.
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.
That is the intent here, if that is an issue for you we should probably discuss that in an issue. If that is a blocker I'd be happy to enable additional properties in the spec and wait for Dan's return to discuss it more in depth
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.
Creating an issue, this is worth discussing either way. I think I'll do a first draft as is and we can update that once that issue progresses.
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.
See #137
91067bb
to
7b91d2e
Compare
spec/data-types-and-values.html
Outdated
1. Assert: Type(_kv_.[[Key]]) is String. | ||
1. Assert: Type(_kv_.[[Value]]) is not Object. | ||
1. If there is no entry _existing_ in _uniqueEntries_ such that _existing_.[[Key]] is _kv_.[[Key]], append _kv_ to _uniqueEntries_. | ||
1. Let _sortedEntries_ be _uniqueEntries_ sorted such that, for any entry two entries _a_ preceding _b_, then the result of performing Abstract Relational Comparison _a_.[[Key]] < _b_.[[Key]] is *true*. |
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.
"for any entry two entries a preceding b" is something i can't get my head to understand, can you help me make sense of it?
also, by using <
, it will return a different value than localeCompare would. is that intentional?
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.
I think using <
would be more accurate than localeCompare as localeCompare would have different results in different locales, what I am trying to express is char code ordering, I will attempt another change to the wording but if that is not good enough to you, is it possible
to suggest something that would express that?
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.
How about:
- Let sortedEntries be uniqueEntries sorted such that, for any two integers i, j less than the length of list uniqueEntries, If i < j then the result of performing Abstract Relational Comparison uniqueEntries[i].[[Key]] < uniqueEntries[j].[[Key]] is true.
Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
Record and Tuple abstract ops
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.
couple of minor copy-paste fixes
Co-authored-by: Rick Button <[email protected]>
…/proposal-record-tuple into spec/record-object-model
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.
approving this and will merge it. we can continue to iterate on this text in subsequent PRs, there will definitely be many more changes after this PR :)
* inital pass of R&T abstract ops * Update spec/data-types-and-values.html Co-authored-by: Jordan Harband <[email protected]> * fix SameValue usage inside of SameValueNonNumeric * reduce verbosity of samevalue checks * added missing reduce/reduceRight * simplify Tuple toString and ToLocaleString * switch to dot notiation for internal slots on records and tuples * equality/comparison operators * Update spec/data-types-and-values.html Co-authored-by: Jordan Harband <[email protected]> * don't create new lists for Record::equal * SameValueNonNumeric -> SameValueNonGeneric * started drafting record exotic object * Define records more in depth * Add RecordCreate * revamp definitions * add RecordTypeCreate * Redo record's define own prop * Apply suggestions from code review Co-authored-by: Jordan Harband <[email protected]> * feedback from Dan * redefine sorting * rework recordcreate * nit: add dots * additional changes * add more internal properties * get through RecordGetOwnProperty * reword Record Exotic Objects intro * Add prevent extensions * refactor internal properties a bit more * fix ! * MakeRecordValue redone * Start properly defining tuple * start structuring tuple section * More structure * advancing tuple * dan's feddback * is/is a * ! SameValue * remove an * Apply suggestions from code review Co-authored-by: Jordan Harband <[email protected]> * first attempt at tuple * adjustments * Fallback to prototype * Remove references to array.prototype * Reword MakeRecordValue again * Apply suggestions from code review Co-authored-by: Jordan Harband <[email protected]> * Apply suggestions from code review Co-authored-by: Jordan Harband <[email protected]> * Rework loop * Use directly Immutable Prototype Exotic Objects * refactor ordering line * Allow symbols to be set on exotics * minor editorial changes * explicit new list * use abstract closure instead of builtin function * Apply suggestions from code review Co-authored-by: Rick Button <[email protected]> * Fix previous bad merge Co-authored-by: Richard button <[email protected]> Co-authored-by: Rick Button <[email protected]> Co-authored-by: Jordan Harband <[email protected]>
No description provided.