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

Overhaul Array documentation #69451

Merged
merged 1 commit into from
Jul 7, 2024

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Dec 1, 2022

Closes godotengine/godot-docs#6964
Closes godotengine/godot-docs#8695
Closes godotengine/godot-docs#9005
Closes #79066
Closes #89276 (properly)
Closes godotengine/godot-docs#9067

Continuation of a series of PRs that go on to touch major, yet majorly unrevised class references. This includes #67072, #67100, #67208, #67718,... #67880 and #68560... and #68649... And I think I'll end up putting all of these in a more organised list at some point.

General

  • Made the wording match how other classes are documented a lot more closely;
  • Made use of [param] and several other strong references more often;
  • Changed words where it felt necessary, where they could be mistaken for another concept of Godot's API, where technical terms are more appropriate. For example:
    • Use "element" consistently.
    • Use "index" consistently.
      • This is not fully possible yet because some parameters are named "position" and it would be confusing.
  • Modified examples extensively, with the main goal to improve readability and variety:
    • Removed quotes around results when they could be misunderstood as strings (# Prints "5" -> # Prints 5);
    • Improved consistency all around;
    • Avoided big walls of print();
    • Added extra padding before comments.
    • Avoid overly long lines.
  • Added slightly more detail to the leading description;
  • Moved the entire "Arrays can be concatenated using the + operator" thing out of the leading description.

Constructors

Array(from)


Methods

bsearch, bsearch_custom

  • Simplified wording considerably.
  • Linked to Binary Search as a bit of an ending note. This is to better understand why the method is named this way, but is not mandatory to know to make use of it.
  • Added bsearch example;

all, any, map, reduce

  • Modified Examples:
  • Show the Callable's definition first.
  • Simplified methods with more familiar terms (negate -> double)

clear

  • "Clears the array"
    • Cool, thanks!

contains

  • Added note about how the check is done by exact Variant type (e.g. 7 is not equal to 7.0).

duplicate

  • Rewrote shallow/deep explanation.

erase, remove_at, insert, pop_at, pop_front

  • "This method acts in-place and doesn't return a value."
    Patronising...
  • Added "Finds and removes" in erase. This slightly extra verbosity implies that the element's index is not known in advance, potentially being slower than removing by index.
  • Simplified the "On large arrays, this method will be slower if the..." note considerably.

fill

  • Modified example to be more "show", less "tell".
  • Rewrote shallow/deep explanation.

find, rfind

hash

is_typed

  • Added static typing example.

set_typed

  • Expanded description of individual parameters (using [br]).

*shuffle

  • "Call randomize to ensure that a new seed will be used each time if you want non-reproducible shuffling."
    In Godot 4, the seed is randomly generated once at start-up, so this is no longer necessary, but the "opposite" is.

*slice

  • Modified wording considerably.
    • This method is such a bother to explain. It does something trivially simple, but the complication is negative parameters being perfectly allowed for some reason.
  • Removed inline examples. They only made the whole thing more difficult to read.
  • Added some examples.

###sort

  • Add more detail to how sorting is performed
  • Move String natural order sorting example out of this description.

sort_custom

  • Modified example to use nested Dictionaries over nested Arrays, to potentially be less confusing while keeping it brief.
    • Reverted until a nicer solution is found, if possible.
  • Moved String natural order sorting example here, previously in sort description.

Operators

+

  • Moved concrete concatenation example here, previously present in the leading description.
  • Explained the performance hit from array concatenation more to the point.

+, >, >=, <, <=

  • Attempted to simplify the explanation considerably.
    • OH BOTHER THIS WAS SUCH A PAIN...

[]


Feedback is very, very welcome.

doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-array-mateys branch from b5341b5 to dd43f78 Compare December 1, 2022 16:11
doc/classes/Array.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-array-mateys branch from dd43f78 to eed30c2 Compare December 1, 2022 16:32
doc/classes/Array.xml Outdated Show resolved Hide resolved
@Calinou Calinou added this to the 4.0 milestone Dec 1, 2022
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-array-mateys branch from eed30c2 to 05247ab Compare December 2, 2022 11:27
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-array-mateys branch from 9a9a678 to 962143e Compare December 5, 2022 17:39
@Mickeon Mickeon marked this pull request as ready for review December 5, 2022 17:44
@Mickeon Mickeon requested a review from a team as a code owner December 5, 2022 17:44
@Mickeon Mickeon force-pushed the doc-peeves-array-mateys branch 3 times, most recently from fbff5d4 to e25de75 Compare December 5, 2022 18:27
@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 10, 2023
@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 8, 2024

Please click on Files changed to see what this PR does and to make accurate comparisons.
image

In this PR, I have already modified the description a loooong while ago, (in an attempt) to be more understandable.

@donn-xx
Copy link

donn-xx commented Mar 8, 2024

Please click on Files changed ...
In this PR, I have already modified the description a loooong while ago, (in an attempt) to be more understandable.

Magic. I didn't know that trick. Thanks. Your description is very good!

@donn-xx
Copy link

donn-xx commented Mar 8, 2024

PS - There is some churn on the examples for bsearch. Here's a link:

#89280 (comment)

@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 8, 2024

I was there 😉 thank you for the reminder though!

@Mickeon Mickeon marked this pull request as draft March 10, 2024 12:07
@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 10, 2024

How's this for an example @AThousandShips @donn-xx ?

func sort_by_amount(a, b):
	if a[1] < b[1]:
		return true
	return false

func _ready():
	var my_items = [["Tomato", 2], ["Kiwi", 5], ["Rice", 9]]
	
	var apple = ["Apple", 5]
	# "Apple" is inserted before "Kiwi".
	my_items.insert(my_items.bsearch_custom(apple, sort_by_amount, true), apple)
	
	var banana = ["Banana", 5]
	# "Banana" is inserted after "Kiwi".
	my_items.insert(my_items.bsearch_custom(banana, sort_by_amount, false), banana)
	
	# Prints [["Tomato", 2], ["Apple", 5], ["Kiwi", 5], ["Banana", 5], ["Rice", 9]]
	print(my_items)

@AThousandShips
Copy link
Member

I think it looks good, not convinced it's needed though, but I'd do:

func sort_by_amount(a, b):
    return a[1] < b[1]

@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 10, 2024

I chose to be explicit as some users do not "wrap around the idea" of comparisons returning bool (It's also consistent with the other example). I guess I could make an exception here? Binary search is not of every-day use.

Also yeah, it's needed (less in this PR because it's better explained). It's not readily apparent and it combines well with sort_custom.

@donn-xx
Copy link

donn-xx commented Mar 10, 2024

How's this for an example

Ah, I see. You pass an array to bsearch_custom. I think the answer is to make it clear that both a and b must be the same types (even though they are accepted as variants).

@AThousandShips
Copy link
Member

AThousandShips commented Mar 10, 2024

both a and b must be the same types (even though they are accepted as variants).

That's not the case, you can search for anything as long as you handle it correctly

In fact I think the one context where it would be good to add an example is to handle that detail, like searching for 5 and not an array element

@Mickeon Mickeon force-pushed the doc-peeves-array-mateys branch from 403833c to 5bbc31d Compare March 10, 2024 16:07
@Mickeon Mickeon marked this pull request as ready for review March 10, 2024 16:07
@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 10, 2024

For the love of the almighty this PR is so old the screenshot taken as a preview doesn't even match how the built-in documentation looks like anymore.

I have expanded upon bsearch's example and bsearch_custom

@donn-xx
Copy link

donn-xx commented Mar 11, 2024

both a and b must be the same types (even though they are accepted as variants).

That's not the case, you can search for anything as long as you handle it correctly

True. I was speaking about practical use. Yeah, I didn't think about probing the argument types within the func and then changing behaviour dynamically.

In fact I think the one context where it would be good to add an example is to handle that detail, like searching for 5 and not an array element

Indeed! This would help.

@Mickeon Mickeon force-pushed the doc-peeves-array-mateys branch from 5bbc31d to a379080 Compare July 5, 2024 10:38
@Mickeon
Copy link
Contributor Author

Mickeon commented Jul 5, 2024

Painfully rebased after #79075

@dalexeev see if you're comfortable with the changes, too. Look for get_typed_class_name get_typed_script, and the typed Array constructor.

doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

I spent the better part of an hour poring over the changes. There are some small parts I could quibble about, but with a change this size, that is always going to be the case. None IMO are important enough to block this further, seeing how long it has been open.

What matters is that overall, this is a large improvement IMO, and none of the few things above matter too much. Overall, this is more clear, more carefully worded and links relevant info. My take is to get this in. :)

@Mickeon Mickeon force-pushed the doc-peeves-array-mateys branch from a379080 to 479e5ef Compare July 5, 2024 11:47
@Mickeon Mickeon force-pushed the doc-peeves-array-mateys branch from 479e5ef to 31a9c63 Compare July 5, 2024 11:49
@akien-mga akien-mga merged commit be9ff1c into godotengine:master Jul 7, 2024
18 checks passed
@akien-mga
Copy link
Member

akien-mga commented Jul 7, 2024

Thanks! Impressive work as usual 🎉

@akien-mga akien-mga changed the title Overhaul Array Documentation Overhaul Array documentation Jul 7, 2024
@Mickeon
Copy link
Contributor Author

Mickeon commented Jul 7, 2024

I have been freed from the curse.

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