Skip to content

feat!: Remove error returned by Hash#1671

Open
alarso16 wants to merge 8 commits intomainfrom
alarso16/remove-hash-error
Open

feat!: Remove error returned by Hash#1671
alarso16 wants to merge 8 commits intomainfrom
alarso16/remove-hash-error

Conversation

@alarso16
Copy link
Contributor

Why this should be merged

I realized that the error returned by the hash function can only happen if the db handle is empty. Otherwise, it will always be nil. Why even give the opportunity to return an error?

How this works

Removes errors from the signature for everything hash-related.

How this was tested

CI

@alarso16 alarso16 force-pushed the alarso16/remove-hash-error branch from 599a32c to 23631fa Compare February 11, 2026 19:59
@alarso16 alarso16 marked this pull request as ready for review February 11, 2026 21:49
defer db.handleLock.RUnlock()
if db.handle == nil {
return EmptyRoot, errDBClosed
return EmptyRoot
Copy link
Contributor Author

@alarso16 alarso16 Feb 11, 2026

Choose a reason for hiding this comment

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

This is the crux of the PR. There are no other possible errors, so it would be really nice to not have deal with this, but it's kind of strange behavior

@demosdemon demosdemon changed the title feat!: Remove error returned by Hash feat(ffi, go)!: Remove error returned by Hash Feb 11, 2026
@demosdemon demosdemon changed the title feat(ffi, go)!: Remove error returned by Hash feat!: Remove error returned by Hash Feb 11, 2026
@demosdemon demosdemon added this to the v0.2.0 milestone Feb 11, 2026
@demosdemon
Copy link
Contributor

Since this is a breaking change for the Go api, we should wait to merge until after 0.1.1 is released.

Copy link
Member

@rkuris rkuris left a comment

Choose a reason for hiding this comment

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

Yeah there's no way to get an error from a revision since the hashes are known before the call is made.

This is a breaking change so let's not merge it until 0.2.0

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.

3 participants