search

rust-bitcoin / rust-bitcoin

Rust Bitcoin library
Creative Commons Zero v1.0 Universal
2.16k stars 701 forks source link

Naming conventions with unwrap #3602

Closed yancyribbens closed 2 weeks ago

yancyribbens commented 3 weeks ago

Is there a precedence for naming methods with unwrap in the title? for example, from_vb_unwrap() seems ad-hoc when compared to the naming conventions used elsewhere in this library. Wouldn't it be more conventional to use unchecked here instead of unwrap?

apoelstra commented 3 weeks ago

I agree that the name _unwrap feels weird and ad-hoc. This method exists for const reasons -- though maybe we don't need it anymore now that we can panic in const contexts? So maybe we can just drop it.

unchecked is definitely not the right suffix for a method that checks its input and panics.

yancyribbens commented 3 weeks ago

unchecked is definitely not the right suffix for a method that checks its input and panics.

Good point. Also directly bellow it there is already an _checked function.

This method exists for const reasons -- though maybe we don't need it anymore now that we can panic in const contexts? So maybe we can just drop it.

Yeah I think it's superfluous really. If this is dropped, pub const fn from_vb_unchecked can still be used in const context.

apoelstra commented 3 weeks ago

True, we can use unchecked in a const context -- but we should also just make from_vb const.

yancyribbens commented 3 weeks ago

but we should also just make from_vb const.

I don't really have an opinion here since I'm not making use of it while in const context, although sure, that could be updated at the same time. I image there's probably lots of other functions that could be const context that are not..