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.

Sign up for GitHub

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

Jump to bottom

Replace all instances of erase with remove #49701

Closed
wants to merge 1 commit into from
Closed

Replace all instances of erase with remove #49701

wants to merge 1 commit into from

Conversation

AaronRecord
Copy link
Contributor

@AaronRecord AaronRecord commented Jun 18, 2021 •
edited
Loading

Array's remove() method was renamed to remove_at()
Dictionary, Array and Set's erase() methods were renamed to remove()

See godotengine/godot-proposals#2885

@AaronRecord AaronRecord requested review from a team as code owners June 18, 2021 03:46
@AaronRecord AaronRecord force-pushed the rename-array-erase-remove branch 3 times, most recently from a9c4952 to 94d92bd Compare June 18, 2021 04:08
@AaronRecord AaronRecord requested a review from a team as a code owner June 18, 2021 04:08
@timothyqiu
Copy link
Member

I think remove_value is a bit long. I wish it's remove(value) and remove_at(index) as named in many other languages / libraries, but it makes it harder to spot error when porting codes from 3.x to 4.0.

@AaronRecord AaronRecord marked this pull request as draft June 18, 2021 13:40
@akien-mga akien-mga removed the request for review from a team June 18, 2021 16:28
@AaronRecord AaronRecord force-pushed the rename-array-erase-remove branch 14 times, most recently from f51c8f7 to a175d41 Compare June 18, 2021 21:11
@AaronRecord AaronRecord mentioned this pull request Jun 18, 2021
Closed
@AaronRecord
Copy link
Contributor Author

AaronRecord commented Jun 18, 2021 •
edited
Loading

I created a proposal (godotengine/godot-proposals#2885).

@AaronRecord AaronRecord force-pushed the rename-array-erase-remove branch 3 times, most recently from 6783eed to f3d2672 Compare June 19, 2021 00:07
@AaronRecord
Copy link
Contributor Author

AaronRecord commented Jun 19, 2021 •
edited
Loading

Okay, based on feedback, I've renamed remove_value() to remove(). I might end of reverting this, but I've also replaced all occurences of erase in method names with remove (including renaming Dictionary's and Set's erase() methods) for consistency (erase was 1/5th as common as remove).
Edit: okay I ended up just replacing all instances of erase with remove (except in thirdparty/). I might've bitten off more than I can chew... we'll see 😄

@AaronRecord AaronRecord force-pushed the rename-array-erase-remove branch from f3d2672 to a4aefb7 Compare June 19, 2021 00:19
@AaronRecord AaronRecord changed the title Array: Rename remove & erase to be more clear Replace all instances of erase with remove Jun 19, 2021
@AaronRecord
Replace all instances of erase with remove
ba0758c 
Array's `remove()` method was renamed to `remove_at()`
@AaronRecord AaronRecord force-pushed the rename-array-erase-remove branch from a4aefb7 to ba0758c Compare June 19, 2021 00:20
@AaronRecord
Copy link
Contributor Author

AaronRecord commented Jun 19, 2021 •
edited
Loading

Okay, I think this would be smarter to do in individual PR's, if this change is desired...

@AaronRecord AaronRecord deleted the rename-array-erase-remove branch June 19, 2021 00:31
@AaronRecord AaronRecord mentioned this pull request Nov 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
archived breaks compat enhancement topic:core
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@AaronRecord @timothyqiu @Calinou @akien-mga