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

Improve panic! messages in hex-literal #664

Merged
merged 4 commits into from Nov 11, 2021

Conversation

thomascastleman
Copy link
Contributor

Thanks for making the hex-literal crate! I was using it with an escaped hex string I'd copied from a C program, but ran into a panic because I'd accidentally left a semicolon after the strings I was calling hex!() on. It took me an embarrassing amount of time to discover the mistake, but I thought the panic messages might be able to provide a bit more info about what caused the failure.

This PR does only a few small things:

  • I added a bit more info to the panic messages when you encounter a non-literal, invalid character, or odd number of hex characters, to give you context about what's causing the issue.
  • I also added some internal documentation on some of the TokenTreeIter functions.
  • Lastly, I added some tests for the core hex!() macro functionality in the tests/ directory (this was the only way I could find to set this up so the tests could invoke hex!()).

I didn't touch the changelog because I figure if this gets merged it's not a substantial enough change to warrant that. Thanks and I'd appreciate any feedback.

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

Thank you, looks good! I have only one small comment.

Also can you please update the changelog with an [UNRELEASED] section?

hex-literal/src/lib.rs Outdated Show resolved Hide resolved
@newpavlov newpavlov merged commit e9a6b98 into RustCrypto:master Nov 11, 2021
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.

None yet

3 participants