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

[Doc] Clarify bsearch(_custom) behavior #89280

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Mar 8, 2024

  • Added an example for the effect of before
  • Clarified the arguments to the custom callable can be either order

Didn't specify that the order of the arguments in the callable is based on the before argument as it's an implementation detail, unlikely to change, but relevant to not lock into


@AThousandShips AThousandShips added enhancement documentation cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 8, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Mar 8, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner March 8, 2024 11:55
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

This can do for now.

Although #69451 would almost supersede this PR with another example outright in the decade it gets merged.

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

AThousandShips commented Mar 8, 2024

Except your example there doesn't showcase the effect of before, so don't remove this example in your PR

@Mickeon
Copy link
Contributor

Mickeon commented Mar 8, 2024

I won't, I'll make sure of it next rebase.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 8, 2024

The original issue was closed by some weird mistake and the example apparently did exist at some point, but the only trace of it I could find is on my private fork. Here's what I could recover:

Finds the index of an existing value (or the insertion index that maintains sorting order, if the value is not yet present in the array) using binary search and a custom comparison method declared in the [code]obj[/code]. Optionally, a [code]before[/code] specifier can be passed. If [code]false[/code], the returned index comes after all existing entries of the value in the array. The custom method receives two arguments (an element from the array and the value searched for) and must return [code]true[/code] if the first argument is less than the second, and return [code]false[/code] otherwise.
				[codeblock]
				func cardinal_to_algebraic(a):
				    match a:
				        "one":
				            return 1
				        "two":
				            return 2
				        "three":
				            return 3
				        "four":
				            return 4
				        _:
				            return 0

				func compare(a, b):
				    return cardinal_to_algebraic(a) < cardinal_to_algebraic(b)

				func _ready():
				    var a = ["one", "two", "three", "four"]
				    # `compare` is defined in this object, so we use `self` as the `obj` parameter.
				    print(a.bsearch_custom("three", self, "compare", true)) # Expected value is 2.
				[/codeblock]

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 8, 2024

I think a more extensive example for bsearch_custom should be done separately, and I'm not convinced it's very useful, it'll just show that you can compare more complexly, but your individual use case is going to be so unique it's not helpful IMO

I can unlink this from the doc issue and someone can add a custom example

But I don't think an example will help to any real degree, either you get what a comparison method is for your specific use case, or you don't, an example isn't going to make it any clearer for you if you don't know how to achieve what you want, IMO

@Mickeon
Copy link
Contributor

Mickeon commented Mar 8, 2024

KoBeWi's example looks nice enough. Outside of nitpicks, the only thing I'd change is the last line, as the comment is superfluous with Callables.

				    print(a.bsearch_custom("three", compare, true)) # Prints 2

@donn-xx
Copy link

donn-xx commented Mar 8, 2024

Please try to make the bit about the before specifier much clearer somehow. Examples are worth a thousands words.

A Thousand Ships tried to explain it to me here. Maybe it's a start.

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 8, 2024

You are linking to a comment by me 🙃 And that's just the code that does this internally, not an example

But as I said above I'm not adding a specific example to that part, i.e. the custom part, given my raeasons above, the example I give shows the behavior as before/after the position, please look, so confused as to why you are asking for an example when one is there?

@donn-xx
Copy link

donn-xx commented Mar 8, 2024

But as I said above I'm not adding a specific example to that part, i.e. the custom part, given my raeasons above, the example I give shows the behavior as before/after the position, please look, so confused as to why you are asking for an example when one is there?

I am confused now. What example? Could be my ignorance. Have checked latest docs online, but see nothing. Am I looking in the right place?

@AThousandShips
Copy link
Member Author

This hasn't been merged so the docs aren't updated, look at the changes in this PR :)

@donn-xx
Copy link

donn-xx commented Mar 8, 2024

This hasn't been merged so the docs aren't updated, look at the changes in this PR :)

If you mean the addition to bsearch:
print(arr.bsearch(3, true)) # Prints 2...

..then I must say that this doesn't help (me) understand that before argument. Why not add an example using false?
(Try to look at docs from the pov of someone who has little time and just wants to use the function quickly.)

doc/classes/Array.xml Outdated Show resolved Hide resolved
@donn-xx
Copy link

donn-xx commented Mar 8, 2024

While we're at it, can anyone write something to explain what this means:

"(or the insertion index that maintains sorting order, if the value is not yet present in the array)"

The sense of it does not leap from the screen. What is an 'insertion index'? What is 'sorting order'? Why would a value not be in the array yet?
😄

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 8, 2024

That's for a future one, see #69451

But that's about how it looks for the place where you could insert a vale, I don't see it as very unclear, but take it over to #69451 and see how it looks for clarity and add your ideas there

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
* Added an example for the effect of `before`
* Clarified the arguments to the custom callable can be either order
@akien-mga akien-mga merged commit ecc4649 into godotengine:master Mar 9, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the arr_order_doc branch March 10, 2024 08:00
@AThousandShips
Copy link
Member Author

Thank you!

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.
Cherry-picked for 4.1.4.

@akien-mga akien-mga removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 11, 2024
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.

bsearch_custom seems to swap arguments depending on "before" argument.
6 participants