Skip to content

Conversation

mattsse
Copy link

@mattsse mattsse commented Nov 20, 2024

in #743 these have been removed in favor of tryfrom/from traits.

this was a breaking change, the into 32 makes it a bit weirder to convert it to an u8. id.to_i32() as u8 is now i32::from(id) as u8.

imo it's reasonable to add them back, so this doesn't break for everyone that hasn't updated yet.

@tcharding
Copy link
Member

Thanks for the contribution. Rust convention is to use From and TryFrom for conversions. I do not think we should add from_i32 back in.

to_i32() however seems useful and IMO probably should not have been removed, we could also add to_u8 if going to a u8 is common.

Both functions can be backported to v0.30.0.

@apoelstra
Copy link
Member

Agreed re from_i32 -- I think we should add it back but deprecate it.

Also agreed that we should add a to_u8 alongside to_i32. This value fits into a u8 and you probably want it in a u8 if your intent is to serialize it.

@apoelstra
Copy link
Member

Indeed, in rust-bitcoin where we use this function we have casts in both directions...when serializing, we do to_i32() as u8 which sucks, and also when deserializing we use as i32 to convert a u8 to a i32, which we shouldn't be doing.

@tcharding
Copy link
Member

tcharding commented Jan 27, 2025

Hi @mattsse , are you working on this still mate? I just bumped into the issue. If you are not I'll push a PR. Thanks

@tcharding tcharding mentioned this pull request Jan 27, 2025
@tcharding
Copy link
Member

Added tracking issue just so this doesn't get lost.

@apoelstra
Copy link
Member

I'm going to close this -- it is untouched for over a year, affects only an obscure and unloved part of the API (which we are going to break anyway in bigger ways with fallout from #806), and we haven't gotten other complaints about this.

We also changed this particular part of the API in #800 and I don't think we intend to double-back and improve the deprecation pathway after so long.

@apoelstra apoelstra closed this Sep 19, 2025
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