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 Array, Dictionary and Variant.duplicate() to copy() #46996

Closed

Conversation

madmiraal
Copy link
Contributor

Does the same internally for Vector<>, which includes all PackedArray types.

Part of #16863.

Note: The original suggestion suggested either copy() or clone() and #16863 actually suggests clone(), but all the documentation uses the work "copy" in the descriptions; so I think it makes more sense to use copy().

@aaronfranke
Copy link
Member

The problem with copy is that it's too imprecise of a word, it can be interpreted as copying a reference, while clone implies a brand new array with the same contents.

@madmiraal
Copy link
Contributor Author

The problem with copy is that it's too imprecise of a word, it can be interpreted as copying a reference, while clone implies a brand new array with the same contents.

I think this is even more reason why copy is a better name for the methods. If you want to clone it i.e. copy the data too, you need to set the 'deep' parameter to true.

@madmiraal
Copy link
Contributor Author

Rebased following 4ca1e73.

@madmiraal madmiraal requested a review from a team as a code owner April 4, 2021 11:40
@madmiraal
Copy link
Contributor Author

Rebased following merge of #46991 and #47166.

@madmiraal
Copy link
Contributor Author

Updated documentation with --doctool ..

@madmiraal
Copy link
Contributor Author

Rebased following 29775a1.

Does the same internally for Vector<>, which includes all PackedArray types.
@akien-mga
Copy link
Member

We discussed this in a PR review meeting today and the consensus was that we didn't see much value in the rename. duplicate() is quite explicit and matches language used in the editor too (Duplicate Node, Duplicate Resource, etc.).

As long as we're consistent in our own API, I think keeping duplicate() is fine.

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.

3 participants