-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add a macro to collect
outputs of a yielding method to an array
#4813
Conversation
I like the idea, but I don't think this should be a global macro, also as
|
Yeah I had a thought like that but assumed it wouldn't work (that macros can be only used unqualified or something) |
Ideally this would be something like |
Well sure, having an iterator for every yielding method would be amazing, but that topic has been neglected, and manually implementing those iterators is no fun. Disagree on your naming ideas. |
src/macros.cr
Outdated
|
||
class Array(T) | ||
macro collect(expr) | ||
arr = [] of typeof(begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no macro variables for the array? We don't want to leak the arr
name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right...
The rest of this is such an eye sore that I didn't realize.
src/macros.cr
Outdated
@@ -83,6 +83,25 @@ macro record(name, *properties) | |||
end | |||
end | |||
|
|||
private record TypeSentinel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private record
does not work, better use struct
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sucks that it doesn't work and doesn't error
Method is great, and |
I'd suggest to add an option to define the generic type of the array directly instead of inferring it as a union of the yielded types: macro collect(type, expr)
%arr = [] of {{type}}
{{expr}} do |x|
%arr << x
end
%arr
end This makes it more clear what array type is returned and allows fine grained control over it. Sometimes you don't want an array of unioned subtypes (or "plus type") but rather explicitly an array of a parent type. |
My previous implementation of this idea had the name "gather" but that's not what it means. It also was more opinionated and wrapped the function instead, so less flexibility, especially for docs. But I can't think of a way to specify the type explicitly like I did there, in a way that doesn't look goofy. |
Wonder if it would be possible to get the type from EDIT: Nope |
@konovod I think the name @straight-shoota No idea honestly how common it would be that people would want to use a different type. Still, that explicit-type version macro is much nicer to read, and seeing what kind of Array will be returned is also nice. So 👍 from me. |
I'm not sure I can like this. Creating an empty array, iterating and pushing values to it is boring and repetitive, but it's descriptive as to what's happening. This solution may look interesting but it's complex to understand and confusing. I prefer alternative methods that return an interator that implements [1, 2, 3].cycle(2).to_a
[1, 2, 3, 4, 5].each_cons(3).to_a |
The dream solution is automatically implementing Iterators based on yielding methods. |
Maybe it'd be easier to understand if you'd have to create the array manually but have a shortcut for adding yielded elements: |
I agree with @ysbaddaden. We already have Iterator#to_a , not sure why we need another style...unless this is providing something different? Personally, I'm not a fan of top-level (or nearly top-level) functions over methods on objects... reminds me of Python's built-in functions, which I don't really like ( |
@@ -1752,8 +1752,7 @@ describe "String" do | |||
end | |||
|
|||
it "works with strings with block" do | |||
res = [] of String | |||
"bla bla ablf".scan("bl") { |s| res << s } | |||
res = Array.collect "bla bla ablf".scan("bl") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular test's purpose is to test scan with a block, so I don't think this one should change (yeah, it's using a block via the collect macro, but that obscures the test itself)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case it is better to be more expressive to understand the purpose.
The reason is that not every method with a block has a corresponding iterator. And even if they do, not all those iterators are the same speed as their block-based counterparts. |
Collecting with blocks should always be faster than |
Ah ok, I think what threw me is that nearly every change in this PR is on methods that already return an iterator (cycle, glob, each_slice, etc). The only one in these changes that doesn't return an iterator is the downcase method. |
it's failed on Travis CI and has merge conflicts. |
Even though I like this, and I'm glad it's possible to do, I'm not sure this should go into the std. I'd like to have general iterators related to |
(closing because of the above comment) |
Just having some fun reducing code duplication.
Obviously this can be improved and documented, I'm just curious about initial feedback.