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

Incorrect typings for Heap#peek #283

Closed
Jannis opened this issue Mar 15, 2021 · 5 comments
Closed

Incorrect typings for Heap#peek #283

Jannis opened this issue Mar 15, 2021 · 5 comments

Comments

@Jannis
Copy link

Jannis commented Mar 15, 2021

The documentation and TypeScript types for Heap#peek say that it always returns T. However, the current implementation returns T | undefined due to it's return this.values[0] implementation:

Code:

peek() {
return this.values[0];
}

Docs: https://docs.thi.ng/umbrella/heaps/classes/_heap_.heap.html#peek

An empty Heap or DHeap would return undefined because emptyArray[0] is undefined.

My suggestion would be to either make peek return T | undefined (preferred due to its better developer ergonomics) or make it throw an exception if there are no values (less preferred), and potentially be more explicit in the documentation.

@earthlyreason
Copy link
Contributor

Related: --noUncheckedIndexedAccess.

Haven't used that myself yet -- I imagine it's pretty breaky. But here, the unsafe signature is leaking into the API, so maybe something to consider.

postspectacular added a commit that referenced this issue Mar 15, 2021
- add explicit return types for Heap.peek()/pop()/pushPop()
postspectacular added a commit that referenced this issue Mar 15, 2021
postspectacular added a commit that referenced this issue Mar 15, 2021
@postspectacular
Copy link
Member

Thanks @Jannis - I'd say the intention here definitely is T | undefined rather than throwing an error (same as Array.peek() contract). There're other parts/packages in umbrella where you will get an OutOfBoundsError, but in those case it's (IMHO) more justified & useful than doing so here. So we're on same page :)

@gavinpc-mindgrub thank you for that option, didn't know about that either, but will check it out!

@Jannis
Copy link
Author

Jannis commented Mar 15, 2021

@postspectacular Awesome, thanks for the quick turnaround!

@postspectacular
Copy link
Member

@Jannis Hah! Don't get used to it :) Will try to do a new release tomorrow...

@postspectacular
Copy link
Member

Just pushed a new release! (changelog) 🎉

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

No branches or pull requests

3 participants