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.invert() to Array.reverse() #46991

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

madmiraal
Copy link
Contributor

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

Part of #16863.

Does the same internally for List and Vector<>, which includes all
PackedArray types.
@madmiraal
Copy link
Contributor Author

Rebased following merge of #47024.

@akien-mga
Copy link
Member

Makes sense to me, that's consistent with Python 3's array.reverse(), and avoids potential confusion with insert().

Anyone against the change?

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.

Seems consensual!

@akien-mga akien-mga merged commit 4b6e9f3 into godotengine:master Apr 1, 2021
@akien-mga
Copy link
Member

Thanks!

@MaaaxiKing
Copy link

MaaaxiKing commented Apr 1, 2021

Does this set or return? I find invert() better if it was set, but not only reverse() but reversed() for a return method without setting it. Comprehensible?

@Calinou
Copy link
Member

Calinou commented Apr 1, 2021

@MaaaxiKing The return type is unchanged – it's still void().

I think we should make this method return a new Array instead. Since renaming the method is a breaking change already, we might as well do it for 4.0. In general, I'd steer clear from methods that modify things in-place as much as possible.

@madmiraal madmiraal deleted the rename-invert-reverse branch April 1, 2021 15:53
@madmiraal
Copy link
Contributor Author

Unlike other ideally immutable types, Arrays are designed to be manipulated "in-place". It would be expensive to create a new array every time it was changed or assigned, which is also why they are passed by reference. Furthermore, when the user does want a copy, they need to specify whether they just want the Array copied or the individual items of the Array too, which is why duplicate(bool deep=false) has a parameter associated with it.

@YuriSizov
Copy link
Contributor

@madmiraal Please use doctool with your renaming PRs. This one broke the alphanumeric order which results in unnecessary changes for others.

@madmiraal
Copy link
Contributor Author

We should probably include it in the CI checks somehow.

@aaronfranke
Copy link
Member

@madmiraal I've been wanting to do this for a long time, but right now doctool has to open a window first, and there's no headless DisplayServer available for Godot as of the DisplayServer split.

@madmiraal
Copy link
Contributor Author

Unless I'm doing something wrong, the doctool doesn't warn about items being out of alphanumeric order.

@akien-mga
Copy link
Member

It doesn't warn but it should fix the order. If there's any diff, then it could fail CI (if we could run it on CI).

@madmiraal
Copy link
Contributor Author

I'm running ed2f51b (i.e. before #47631) with --doctool and it doesn't fix the alphanumeric order. In doc/classes/Array.xml, <method name="reverse"> is still before <method name="is_empty" qualifiers="const"> 😕

@akien-mga
Copy link
Member

akien-mga commented Apr 5, 2021

Are you running with bin/godot --doctool . (with the .)?
If so it might be a system-specific issue, in the past doctool produced inconsistent ordering on Windows but I thought this has been fixed.

@madmiraal
Copy link
Contributor Author

I was not. It works when I include the .. 🎉 😃

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.

None yet

6 participants