-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Proposal of new PropertyDescriptor
design
#1432
Conversation
PropertyDescriptor
design PropertyDescriptor
design
Fixed tests:
|
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.
Looks good
Check my comments to see on how we might improve this :)
86dbc2e
to
5d83811
Compare
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.
Looks good to me :)
This just needs a rebase so we can merge :) |
5d83811
to
122d7b7
Compare
Rebased. Still, I'm gonna check if there are no failing tests caused by the rebase. |
Everything should be OK. We can merge now :) |
It seams after rebase we got another 2 passing tests :)
Fixed tests:
|
This PR changes
PropertyDescriptor
to allow for generic descriptors. Opted for a builder pattern because using alwaysSome(val)
was very tedious. The type system ensures that you cannot create an invalid descriptor, since the builder automatically switches to a valid descriptor type when you specify[[writable]]
,[[value]]
,[[get]]
or[[set]]
, but we could make it panic instead if needed.This design follows the spec more closely, so I expected to see a bump on the number of passed tests. Fortunately, running the 262 test suite shows that the number of passed tests increase from 28322 to 29109.
closes #1426