Skip to content

Conversation

@reinux
Copy link

@reinux reinux commented Apr 20, 2023

@dsyme
Copy link
Contributor

dsyme commented Jun 12, 2024

@KevinRansom @vzarytovskii @abelbraaksma Comments on this please?

@T-Gro
Copy link
Contributor

T-Gro commented Jun 17, 2024

@reinux :

I am still thinking of a better self-explainable API, I don't think a tuple of 3 with same types is best. Even though the ordering (previous,match,next) is likely the most natural one here.

Just to show another proposal, I have tried to DU-model the outcomes, but I do not think it is any better to your proposal here. The number of possible combinations is an inherent part of binary search if we want this to be a primitive supporting all sorts of scenarios.

type Node<'TKey,'TValue> = (struct ('TKey * 'TValue))

[<Struct>]
type MapPosition<'a,'b> =  
    | Between of below:Node<'a,'b> * above:Node<'a,'b>
    | AtMinimum of above:Node<'a,'b>
    | AtMaximum of below:Node<'a,'b>
    | MapWasEMpty

[<Struct>]
type BinarySearchResult<'a,'b> =
    | ExactMatch of hit:Node<'a,'b> * position:MapPosition<'a,'b> 
    | NotFound of  position:MapPosition<'a,'b> 

@Martin521
Copy link
Contributor

Martin521 commented Aug 13, 2024

I like the functionality and the API.
I am not sure about the name. "Binary search" is a well defined algorithm for arrays with a single return value.
Just Map.search? Map.tryFindWithNeighbors?

@reinux
Copy link
Author

reinux commented Sep 21, 2024

I like the functionality and the API. I am not sure about the name. "Binary search" is a well defined algorithm for arrays with a single return value. Just Map.search? Map.tryFindWithNeighbors?

The more I think about it, the more I'm liking Map.search. Even if I could validly argue that it's a special case of binary search (and I'm starting to have doubts), you probably wouldn't be the only one raising an eyebrow.

search is a nice, short name, and the return type, whatever we end up landing on, will do a fine job describing what it does anyway.

As for the return type, I think the only option we haven't explored yet is a single-case union with named parameters:

type MapSearchResult<'k, 'v> =
  MapSearchResult of left: ('k, 'v) option * exact: ('k, 'v) option * right: ('k, 'v) option

This would effectively be the same as the tuple version, except that we'd be trading conciseness with some IDE assistance.

I'm likewise unsure whether this is the best approach either.

@T-Gro
Copy link
Contributor

T-Gro commented Feb 12, 2025

From the options so far, this API seems the best for avoiding errors and for communicating intent clearly.

type MapSearchResult<'k, 'v> =
  MapSearchResult of left: ('k, 'v) option * exact: ('k, 'v) option * right: ('k, 'v) option

Map.search is fitting

@Happypig375
Copy link
Contributor

The MapSearchResult return type seems like a workaround for the lack of named tuple fields in F#.

@T-Gro
Copy link
Contributor

T-Gro commented Jul 29, 2025

There are more benefits to a custom type over tuples for API contracts:

  • Binary stability in the case of more fields added in the future (whereas going from 3-tuple to e.g. 4-tuple is a binary breaking change)
  • Understood by older compilers and/or IDE tools
  • Can have extensions defined

@Happypig375
Copy link
Contributor

Happypig375 commented Aug 10, 2025

@T-Gro A binary breaking change would still occur when adding another field to MapSearchResult on the construction side, even though deconstruction, based on member access, wouldn't break.

The point about older compilers and IDE tools may apply but documentation would help - we can't be stuck in the past with API additions.

Also, are extensions really a point of concern? What are the use cases for extending MapSearchResult at all, as opposed to using a function for it?

@T-Gro
Copy link
Contributor

T-Gro commented Aug 11, 2025

Also, are extensions really a point of concern? What are the use cases for extending MapSearchResult at all, as opposed to using a function for it?

members on the type can be a useful API tool, especially for teams used to dot-driven-development. ( e.g. .IsEmpty)

Copy link
Contributor

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

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

See #734 (comment) for the last changes to incorporate.
After that is done, this PR can be merged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants