Skip to content
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

Fix getMethod #6

Closed
wants to merge 5 commits into from
Closed

Fix getMethod #6

wants to merge 5 commits into from

Conversation

UltCombo
Copy link
Collaborator

@domenic
Copy link
Owner

domenic commented Jul 19, 2015

Tests showing the difference?

@UltCombo
Copy link
Collaborator Author

Added test.

@UltCombo
Copy link
Collaborator Author

I can squash the commits if you'd like and if it is okay for merging.

@domenic
Copy link
Owner

domenic commented Jul 19, 2015

Can you test Get vs. GetV?

@UltCombo
Copy link
Collaborator Author

Yes, but I just realized something odd. With this change, this test should not be passing anymore. Oh, that's because this assertion must be removed as well.

@UltCombo
Copy link
Collaborator Author

PTAL.

@UltCombo
Copy link
Collaborator Author

The only difference I see is that Get would throw an error for non-objects while GetV boxes them.
As GetMethod does an IsPropertyKey assertion before calling GetV, there's no observable difference between GetV and [[Get]] in this case, as far as I can see.

@UltCombo
Copy link
Collaborator Author

Just to be sure, when you asked for Get vs. GetV, you actually mean [[Get]] vs. GetV? There's no observable change as mentioned above.
The abstract operation Get was never part of the code.

@UltCombo
Copy link
Collaborator Author

Does this need anything else to be merged? @domenic

@domenic
Copy link
Owner

domenic commented Jul 21, 2015

No, it looks good. I just need to get some free time to merge and release, which is a bit tricky as I'm traveling for standards meetings the next two days. Maybe I can pull it off on this bus ride though...

@domenic
Copy link
Owner

domenic commented Jul 21, 2015

Merged as 523059e; 2.0.1 coming shortly.

@domenic domenic closed this Jul 21, 2015
@UltCombo UltCombo deleted the patch-1 branch July 21, 2015 14:24
@UltCombo
Copy link
Collaborator Author

Thanks!

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.

None yet

2 participants