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

Rename remove() to remove_at() when removing by index #50139

Merged

Conversation

AaronRecord
Copy link
Contributor

@AaronRecord AaronRecord commented Jul 3, 2021

@AaronRecord AaronRecord force-pushed the rename-remove-to-remove-at branch 3 times, most recently from cac170f to e993888 Compare July 4, 2021 17:39
@Calinou Calinou added this to the 4.0 milestone Jul 4, 2021
@AaronRecord AaronRecord force-pushed the rename-remove-to-remove-at branch 5 times, most recently from 154aa85 to e60a2d4 Compare July 4, 2021 18:36
@AaronRecord AaronRecord changed the title Array/Vector: Rename remove to remove_at Rename remove() to remove_at() when removing by index Jul 4, 2021
@AaronRecord AaronRecord force-pushed the rename-remove-to-remove-at branch 6 times, most recently from bf3c5d6 to 8784605 Compare July 4, 2021 20:04
@AaronRecord AaronRecord marked this pull request as ready for review July 4, 2021 20:44
@AaronRecord AaronRecord force-pushed the rename-remove-to-remove-at branch from 8784605 to e533e37 Compare July 4, 2021 20:44
@AaronRecord AaronRecord requested review from a team as code owners July 4, 2021 20:44
@AaronRecord AaronRecord force-pushed the rename-remove-to-remove-at branch 6 times, most recently from 71f0203 to 4b4fae9 Compare September 3, 2021 15:50
@AaronRecord
Copy link
Contributor Author

Rebased. There's still a number of methods (e.g. https://github.com/godotengine/godot/blob/master/editor/editor_data.cpp#L538, remove.+?\(int (p_)?[_a-zA-Z0-9]+?in?de?x) that could have _at appended to them, but I've chosen to only focus on the array/vector methods to keep this PR's scope more reasonable. If desired though, I could rename those methods, or they could be done in a separate PR etc.

@AaronRecord AaronRecord force-pushed the rename-remove-to-remove-at branch 7 times, most recently from 9489e6e to b2a9d5a Compare September 9, 2021 00:45
@AaronRecord
Copy link
Contributor Author

AaronRecord commented Sep 9, 2021

(rebased) @aaronfranke

Also I wonder if Array.insert(...) should be renamed to insert_at(...) for consistency. I think that'd probably make sense, but it should be done in a separate PR.
Edit: Actually insert(...) is probably fine as is, as I think the name is mostly self-explanatory.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

I think this is a good idea.

@akien-mga
Copy link
Member

This was approved in PR review meeting (sorry for the delay), it just needs a rebase (or redo if that's easier).

@AaronRecord AaronRecord force-pushed the rename-remove-to-remove-at branch from b2a9d5a to 2ba7a54 Compare November 24, 2021 01:47
@AaronRecord AaronRecord requested a review from a team as a code owner November 24, 2021 01:47
@AaronRecord
Copy link
Contributor Author

AaronRecord commented Nov 24, 2021

rebased.

@AaronRecord AaronRecord force-pushed the rename-remove-to-remove-at branch from 2ba7a54 to e078f97 Compare November 24, 2021 01:59
@akien-mga akien-mga merged commit 96e70ac into godotengine:master Nov 24, 2021
@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.

Array & Vector: rename remove() to remove_at(), Array, Dictionary & Set: rename erase() to remove()
7 participants