godotengine
Add callable support for `find`, `rfind`, and `count` `Array` methods
I have implemented a number of methods for using callables with:
find(find_custom)rfind(rfind_custom)count(count_cusom)
I implemented these in a previous PR, but I accidentally deleted it when cleaning up my github, so I am doing it again. Also, I am better at contributing to codebases now, and working with C++.
On the last PR, count_custom could just be reduce, and indeed that's what it uses internally, but I would argue that this is okay, as there is precedent for using simple wrapper methods to avoid confusion with less experienced programmers and work with their intuition (has relies on find, append is an alias for push_back). On the other hand, instead of max_custom and min _custom, you're instructed to use reduce instead, so there is precedence against, too.
Looks like I need to write tests. Also, I think my count_custom implementation using reduce is problematic. I can hand-roll it, or remove it entirely.
@AThousandShips resolved. Good catch. Also realized that _count_reduce wouldn't have actually done anything, so I fixed that.
A few remaining questions:
- Where should I put tests for these?
- The CI build keeps failing on linux-mono, for some reason involving the doc tool. I don't totally understand the failure here.
- I briefly implemented a
take_whilemethod before realizing that it could be simplified tofind_custom+slice. Should I add that back in, along with any more functional programming-style methods? I can seeskip(creating a new array made of everynth element of the source) being useful, and there's also things likezipandunzip. Could be out of the scope of the PR, though.
Where should I put tests for these?
Here tests/core/variant/test_array.h
Once you've fixed the callable argument issue we can see what the doc things are
Where should I put tests for these?
Here
tests/core/variant/test_array.hOnce you've fixed the callable argument issue we can see what the doc things are
I fixed the const issue. I was going to put the test in there, but I didn't see one for filter, so I wasn't sure if there was a special place I couldn't find. I'll add the tests now.
I've added the tests, and fixed the bind method. Good catch. Makes sense that that failed.
Escaping XML characters is a secret art and even highly experienced developers here (like myself) struggle with them 🤣
The core team, but I'll give this my approval as well once I've tested it out a bit and gone over it, we'll hopefully release 4.3 soon and then this can get attention
Will go over it but again the core team needs to review, and I'm not on the core team, will leave a final review once I've gone over it but I'll wait for the core team to give their opinion first
Alrighty. I hope they get to it eventually... I'm sure they are swamped so I won't bother them.
On the last PR,
count_customcould just bereduce, and indeed that's what it uses internally, but I would argue that this is okay, as there is precedent for using simple wrapper methods to avoid confusion with less experienced programmers and work with their intuition (hasrelies onfind,appendis an alias forpush_back).
I personally still don't see much of an use in count_custom, either. But I can also understand it may need to be there for completion sake. Not sure how strong the argument about less experienced programmers is, given that passing Callables around is not exactly a beginner-friendly thing to do. Most beginners would rather do it in a for loop.
@Mickeon
That's true, count_custom is more or less just a wrapper over reduce. And I suppose passing around a Callable might not be easy for beginners. "Reduce" is the functional programming name for it, but I believe the name doesn't really communicate what it's capable of for people who haven't used it before, nor does the example. (I know because this was me last year.)
A compromise could be to add an example to use reduce to count, and adding a note to count that you can use reduce to accomplish the effect of count_custom.
The only real benefit of count_custom is a marginal performance boost over using for in GDScript
Did you mean to restore the code for count_custom? You didn't bind it so either it should be bound or not implemented
In the future keep in mind you can apply suggestions as a batch, please do so in the future to not force many reruns of the CI (and unnecessary notifications)
Also later before merger the commit title should be changed to match the PR title to make it clearer
if I were to build on this and add more array functions in a future PR, would that have to go through proposals first?
Message ID: @.***>