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

Implement Symbol type when the Symbol global is available. #262

Merged
merged 5 commits into from
Feb 11, 2016

Conversation

papandreou
Copy link
Member

Fixes #261.

Symbols can also be used as keys in objects -- that's not implemented yet.

@sunesimonsen
Copy link
Member

Cool but CI fails.

@sunesimonsen
Copy link
Member

It is just some listing errors.

@papandreou
Copy link
Member Author

Yeah, seems like our jshint does not know that Symbol should be an exception to the "all identifiers starting with a capital letter are constructors and must invoked with new" rule. Maybe we can fix it by upgrading, but we might have to specify esversion: 6 for it to work. I'll look into it.

@sunesimonsen
Copy link
Member

Linting is just never fun, tried to upgrade standard today and got 100+ errors :-S

@papandreou
Copy link
Member Author

I think I sorted it out now.

Also added support for Symbols as keys in objects. Even though I'm not totally satisfied with how that turned out, I think we'll need to include it at some point for completeness. Let me know what you think.

@sunesimonsen
Copy link
Member

Very nice 👍

@papandreou
Copy link
Member Author

Raising the coverage further is tricky. The keyComparator doesn't get fully exercised because getKeys returns the keys followed by the symbols.

papandreou added a commit that referenced this pull request Feb 11, 2016
Implement Symbol type when the Symbol global is available.
@papandreou papandreou merged commit 3476465 into master Feb 11, 2016
@papandreou papandreou deleted the feature/Symbol branch February 11, 2016 21:59
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.

2 participants