Skip to content

Implement Array#to_h#901

Merged
elia merged 1 commit intoopal:masterfrom
vais:array-to_h
May 30, 2015
Merged

Implement Array#to_h#901
elia merged 1 commit intoopal:masterfrom
vais:array-to_h

Conversation

@vais
Copy link
Copy Markdown
Contributor

@vais vais commented May 30, 2015

No description provided.

@elia
Copy link
Copy Markdown
Member

elia commented May 30, 2015

Keys seem to be passed directly as keys of the map but I think they could be of any type.

IIRC hash2 is just for hashes with string keys as an optimization for the compiler.

@vais
Copy link
Copy Markdown
Contributor Author

vais commented May 30, 2015

@elia oops, I did not think of that. What would you recommend? Is there any other recommended "fast" way to make a Ruby hash out of a JS one?

@elia
Copy link
Copy Markdown
Member

elia commented May 30, 2015

Try with Opal.hash but check because I'm not sure if it takes also an array of arrays

@vais
Copy link
Copy Markdown
Contributor Author

vais commented May 30, 2015

@elia I'm also looking into making Opal.hash pass all the Hash.[] specs BTW.

I think this one is good for now with using a ruby hash and calling store on it, I did not observe any significant performance difference of this vs the original using Opal.hash2.

Any objections to merging this updated PR at this point?

@vais
Copy link
Copy Markdown
Contributor Author

vais commented May 30, 2015

Also, @elia, @meh, in your expert opinion, from JS code, is it better to:

  1. hash = #{{}} or hash = Opal.hash() to create a ruby hash? Does it matter?
  2. hash.$store(key, val) or #{hash.store(key, val)} to store a value? Does it matter?

I mean in terms of what would be considered more idiomatic and maintainable in the long run.

elia added a commit that referenced this pull request May 30, 2015
@elia elia merged commit 4a43768 into opal:master May 30, 2015
@elia
Copy link
Copy Markdown
Member

elia commented May 30, 2015

I prefer {} and I see it as more maintainable

But no strong feelings on this

@vais vais deleted the array-to_h branch May 30, 2015 11:46
@vais
Copy link
Copy Markdown
Contributor Author

vais commented May 30, 2015

Thanks, @elia

hmdne pushed a commit to hmdne/opal that referenced this pull request Jan 27, 2024
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