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

Add nth unit tests #32

Closed
wants to merge 2 commits into from
Closed

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Oct 25, 2023

Add some unit tests to prove that the default implementation of nth works correctly for HexToBytesIter and BytesToHexIter.

  • Patch 1: Fixes clippy warnings introduced by a new version of clippy.
  • Patch 2: Adds unit tests.

Resolve: #14

@tcharding tcharding force-pushed the 10-25-nth branch 2 times, most recently from 763bece to 38cad31 Compare October 25, 2023 04:40
@apoelstra
Copy link
Member

CI is broken because of another MSRV break in our sole dependency.

@tcharding
Copy link
Member Author

tcharding commented Oct 25, 2023

Thanks for checking the ci fail for me, I added a pinning patch at the front of the PR. No other changes.

Clippy emits:

 error: useless use of `vec!`

As suggested, remove the macro call. Rename the variable to `bytes`
instead of `v` to better suit the new array type.
Add some unit tests to prove that the default implementation of `nth`
works correctly for `HexToBytesIter` and `BytesToHexIter`.

Resolve: rust-bitcoin#14
@tcharding
Copy link
Member Author

Needs #33 to get past CI.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK e2fd51a

@tcharding
Copy link
Member Author

This PR is total rubbish because it misses the whole point that implementing a custom nth is a performance thing not a functionality thing - face palm.

@tcharding tcharding closed this Oct 25, 2023
@apoelstra
Copy link
Member

Yes, but surely there's still value in testing that the perf improvements don't break the functionality?

@tcharding
Copy link
Member Author

True, I might have been a bit trigger happy with the close button. We definitely should put unit tests in the perf PR.

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.

Implement nth and nth_back
2 participants