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

Loop in a more rubyish way in Readme #14

Merged
merged 1 commit into from
Jun 21, 2019
Merged

Conversation

dnsco
Copy link
Contributor

@dnsco dnsco commented Jun 12, 2019

Ruby devs generally iterate using the more functional methods rather than setting up an index and mutating it.
If this they are the target audience of this doc this would probably be clearer.

Ruby devs generally iterate using the more functional methods rather than setting up an index and mutating it.
If this they are the target audience of this doc this would probably be clearer.
@irxground
Copy link
Contributor

Currently, MemoryView does not have an each method, so we can not write such code.

I tried to implement each method, but I can not judge whether this is suitable for MemoryView.

@dnsco
Copy link
Contributor Author

dnsco commented Jun 13, 2019

Sorry, I (wrongly) assumed that memory view was just a ruby array with the data.

It could be nice to define an each and include the enumerable so that transforms of the data could be defined in terms of map/reduce, but I also understand why you could have concerns around it.

@irxground
Copy link
Contributor

MemoryView is a large general purpose buffer, so I'm worried about the need for methods like each to iterate from start to end.

@Hywan, could you tell us your opinion?

@Hywan
Copy link
Contributor

Hywan commented Jun 14, 2019

I'm not familiar with Ruby idiomatic API to be totally honest.

I like this patch irxground@eeaf9f7. I think it makes sense and it feels more idiomatic.

About .include?, I guess it's fine to implement it too. I've no particular opinion against. Wouldn't it be possible to achieve that with a slice though? Anyway, go for it, I'm OK!

@Hywan Hywan self-assigned this Jun 14, 2019
@Hywan Hywan added 🎉 enhancement New feature or request 📦 component-extension About the Ruby extension written in Rust labels Jun 14, 2019
@irxground
Copy link
Contributor

Thank you @Hywan. I will send a PR later.

bors bot added a commit that referenced this pull request Jun 17, 2019
15: Enhance `Wasmer::XxxArray` r=Hywan a=irxground

ref: #14
Now `Wasmer::XxxArray` has `each` method and include `Enumerable` module.

> The Enumerable mixin provides collection classes with several traversal and searching methods, and with the ability to sort. The class must provide a method each, which yields successive members of the collection.
https://docs.ruby-lang.org/en/2.6.0/Enumerable.html


Co-authored-by: irxground <irxnjhtchlnrw@gmail.com>
@Hywan
Copy link
Contributor

Hywan commented Jun 18, 2019

Can we close this in favor or #15?

@irxground
Copy link
Contributor

This pull request is a modification of the README. PR #15 does not include that.

@dnsco
Copy link
Contributor Author

dnsco commented Jun 19, 2019

@Hywan this is a doc change that was incorrect until @irxground implemented what it is documenting in #15.

My personal take is this is more clear documentation for ruby devs, especially now that it is actually implemented ;-)

Thanks again @irxground for figuring out the impl!

@Hywan
Copy link
Contributor

Hywan commented Jun 21, 2019

bors r+

@Hywan
Copy link
Contributor

Hywan commented Jun 21, 2019

You're right, let's go :-).

bors bot added a commit that referenced this pull request Jun 21, 2019
14: Loop in a more rubyish way in Readme r=Hywan a=denniscollective

Ruby devs generally iterate using the more functional methods rather than setting up an index and mutating it.
If this they are the target audience of this doc this would probably be clearer.

Co-authored-by: Dennis Collinson <dennis.collective@gmail.com>
@Hywan Hywan added 📚 documentation Do you like to read? and removed 📦 component-extension About the Ruby extension written in Rust labels Jun 21, 2019
@bors
Copy link
Contributor

bors bot commented Jun 21, 2019

Build succeeded

@bors bors bot merged commit ca86a69 into wasmerio:master Jun 21, 2019
@Hywan
Copy link
Contributor

Hywan commented Jun 21, 2019

Thanks!

@dnsco
Copy link
Contributor Author

dnsco commented Jun 21, 2019

Thank you for merging my feature-request-as-documentation ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 documentation Do you like to read? 🎉 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants