Skip to content

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Jul 31, 2024

Turns out clippy does not warn for unused generics.

Post merge edit for when we write changlog: API breaking change.

Fix: #191

@tcharding tcharding marked this pull request as draft July 31, 2024 19:41
Copy link
Member

@clarkmoody clarkmoody left a comment

Choose a reason for hiding this comment

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

ACK d0905b0

@tcharding tcharding force-pushed the 07-31-rm-unused-generic branch from d0905b0 to 725df12 Compare August 6, 2024 02:49
@tcharding
Copy link
Member Author

Force push is rebase only, will still need another one once #192 merges.

Turns out `clippy` does not warn for unused generics.

Fix: rust-bitcoin#191
@tcharding tcharding force-pushed the 07-31-rm-unused-generic branch from 725df12 to 7bdabff Compare August 6, 2024 22:28
@tcharding
Copy link
Member Author

Rebase only, no code changes.

@tcharding tcharding marked this pull request as ready for review August 6, 2024 22:28
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 7bdabff successfully ran local tests; note that this is technically a breaking change

@apoelstra apoelstra merged commit 7e03432 into rust-bitcoin:master Aug 6, 2024
@apoelstra apoelstra deleted the 07-31-rm-unused-generic branch August 6, 2024 23:40
@stevenroose
Copy link
Contributor

@tcharding why was that unused argument added there? very confused by this

@apoelstra
Copy link
Member

I think this was my code, and the reason is that an early iteration of this API had a ton of generic iterators. The idea was that you could turn any iterator over bytes into one over Fes, any one over Fes into one that added a checksum, etc, etc.

But it turned out that the resulting API was super confusing and undiscoverable (and sometimes impossible to implement because of stdlib bugs (ExactSizeIterator not being implemented for a chain of exact size iterators) and/or limitations in Rust generics (cannot the type produced by map)) and didn't even provide any value.

So I switched to this API with specific iterator types that have to be called in a specific order and things are simpler.

@apoelstra
Copy link
Member

apoelstra commented Jul 25, 2025

Oh, wait. Your question is about this PR which replaces the unused generic (which I explained above) with an unused u8 parameter.

Cut/paste error? Very strange. @tcharding

Which I didn't see, and I think Clark didn't see either, because the diff looks so much like line noise..

@tcharding tcharding mentioned this pull request Jul 26, 2025
@tcharding
Copy link
Member Author

@tcharding why was that unused argument added there? very confused by this

Ha, by looking at the PR I have no idea what so ever. #222

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.

CheckedHrpstring::fe32_iter has an unused generic

4 participants