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

Add callable support for find and rfind Array methods #95449

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

SlashScreen
Copy link
Contributor

@SlashScreen SlashScreen commented Aug 12, 2024

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.

Closes godotengine/godot-proposals#6889

@SlashScreen SlashScreen requested review from a team as code owners August 12, 2024 21:52
@clayjohn clayjohn added this to the 4.x milestone Aug 12, 2024
@SlashScreen
Copy link
Contributor Author

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.

@SlashScreen
Copy link
Contributor Author

Where are tests for sort_custom written?

@SlashScreen SlashScreen force-pushed the array_functions branch 3 times, most recently from c4cda77 to 78a8275 Compare August 13, 2024 03:32
@AThousandShips AThousandShips changed the title Implemented custom array methods Add callable support for find, rfind, and count Array methods Aug 13, 2024
@SlashScreen
Copy link
Contributor Author

@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_while method before realizing that it could be simplified to find_custom + slice. Should I add that back in, along with any more functional programming-style methods? I can see skip (creating a new array made of every nth element of the source) being useful, and there's also things like zip and unzip. Could be out of the scope of the PR, though.

@AThousandShips
Copy link
Member

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

@SlashScreen
Copy link
Contributor Author

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

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.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fix CI

@SlashScreen SlashScreen requested a review from a team as a code owner August 13, 2024 15:29
@SlashScreen
Copy link
Contributor Author

I've added the tests, and fixed the bind method. Good catch. Makes sense that that failed.

@SlashScreen
Copy link
Contributor Author

Sorry, I caught a typo.

@SlashScreen
Copy link
Contributor Author

Tests run during CI, right?

@SlashScreen
Copy link
Contributor Author

If I added these changes, would I have to go through the approval process again?...

@KoBeWi
Copy link
Member

KoBeWi commented Sep 7, 2024

No.
Also this still needs approval from core team anyway. I implemented similar methods before, so I can tell whether the code is ok, but core team has final saying whether the feature should be added (although it's a formality at this point).

@SlashScreen
Copy link
Contributor Author

The CI says there's a duplicate declaration of Array? What?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 7, 2024

You accidentally pushed some wrong file
image

@SlashScreen
Copy link
Contributor Author

oops.

@SlashScreen
Copy link
Contributor Author

@Nazarwadim done.

@dalexeev
Copy link
Member

To me count_custom() makes sense. Of course, reduce() can be used instead, but it is less convenient than a unary predicate. reduce() is a general function, and count_custom() is a common special case, in my opinion.

@SlashScreen
Copy link
Contributor Author

To me count_custom() makes sense. Of course, reduce() can be used instead, but it is less convenient than a unary predicate. reduce() is a general function, and count_custom() is a common special case, in my opinion.

I agree, but the consensus seemed to be to remove it.

@SlashScreen
Copy link
Contributor Author

@Nazarwadim @dalexeev done.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have significant demand as there's been already at least 3 PRs attempting to implement this, and the linked proposal is somewhat consensual.

The proposed API seems fine to me. I didn't review implementation details but others did, so I think this is good to go.

@akien-mga akien-mga merged commit 6bf8a3e into godotengine:master Sep 20, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more array methods: first_custom, last_custom, count_custom
8 participants