Skip to content

Conversation

akroshg
Copy link
Contributor

@akroshg akroshg commented Jan 21, 2016

These ES7 features are implemented as per the ES7 proposal https://github.com/tc39/proposal-object-values-entries/blob/master/spec.md. The implementation is predominantly based on the result of the Object.keys. unittest is added for more information about the usage.

@msftclas
Copy link

Hi @akroshg, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Akrosh Gandhi). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@ljharb
Copy link
Collaborator

ljharb commented Jan 21, 2016

LGTM from a compliance angle :-)

@akroshg
Copy link
Contributor Author

akroshg commented Jan 21, 2016

thanks @ljharb.
@abchatra - could you take a look at this one?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume we have a non-enumerable property when this call is made. Between here and the IsEnumerable call on line 1134, it looks like it's possible to make the property enumerable. Wouldn't it be excluded from the result?

I see that we have tests for the opposite direction (i.e., enumerable -> non-enumerable), but we're missing this case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - the spec says that the list of existing own keys is fixed at the initial call, but that means a preexisting own key's enumerability can change in either direction during enumeration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goyakin, I'll get back to you on this. It is good test case to add.

@akroshg
Copy link
Contributor Author

akroshg commented Jan 23, 2016

@goyakin Yes there was issue in making non-enumerable to enumerable while iteration. I have fixed that and added the test cases.

@chakrabot chakrabot merged commit 83c8a12 into chakra-core:master Jan 26, 2016
chakrabot pushed a commit that referenced this pull request Jan 26, 2016
Merge pull request #171 from akroshg:values
These ES7 features are implemented as per the ES7 proposal https://github.com/tc39/proposal-object-values-entries/blob/master/spec.md. The implementation is predominantly based on the result of the Object.keys.  unittest is added for more information about the usage.
@ljharb
Copy link
Collaborator

ljharb commented Jan 26, 2016

( ゚◡゚)/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants