Skip to content

Conversation

@qnikst
Copy link
Contributor

@qnikst qnikst commented Nov 7, 2025

Slightly improves error messages in case if check fails. It’s still far from the optimal ones but at least mentions the offset where an error happens

@qnikst qnikst requested a review from Copilot November 7, 2025 14:14
@qnikst qnikst self-assigned this Nov 7, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@qnikst qnikst requested a review from Copilot November 10, 2025 13:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@qnikst qnikst requested a review from joaosreis November 12, 2025 14:30
case CBOR.deserialiseFromBytesWithSize CBOR.decodeTerm (BSL.fromStrict bytes) of
Left err -> failUnpack $ TextError $ "CBOR term deserialisation failed: " <> T.pack (show err)
Right (_rest, bytesRead, term) -> do
put (start + fromIntegral bytesRead)
Copy link
Member

Choose a reason for hiding this comment

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

What's the impact of dropping this line? It doesn't seem related to the error message change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question I think I'll close this PR. And prepare another one

I wanted to write that this line is noop and that's true if everything is correct, however in case if not entire cbor structure is consumed there is a difference, and I think we should just fail here in that case

@qnikst
Copy link
Contributor Author

qnikst commented Nov 13, 2025

Converting to draft because the question @nc6 asked opens a deeper issue that should be resolved

@qnikst qnikst marked this pull request as draft November 13, 2025 16:40
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