paritytech / parity-common

Collection of crates used in Parity projects
https://www.paritytech.io/
Apache License 2.0
282 stars 213 forks source link

Remove From [u8; n] impl for uint types #859

Closed pgherveou closed 6 days ago

pgherveou commented 1 week ago

Changes:

pgherveou commented 6 days ago

Thanks. The From implementation already bit me when converting from little endian.

I am not sure of we should also bump the version in this PR or if this is done at release time.

Looks like bumping is done in releases, I can update the changelogs once this is approved by owners

athei commented 6 days ago

FWIW the alloy_primitives U256 doesn't support this conversion neither (although I don't know the exact reason).

Implementing From where the conversion isn't obvious is against the API guidelines. See the last point here. They even mention bytes to number conversion as an explicit example where to not implement it.

ordian commented 6 days ago

Looks like bumping is done in releases, I can update the changelogs once this is approved by owners

Updating the changelogs should ideally happen in the PR itself. If you want to release a new version straight away, you can also bump the versions here. Note that this breaking change will propagate to other crates like primitive-types. It's also a good practice to write how to migrate to a new version in the changelog (this method removed, use that instead).

Releasing a new version then should be easy: https://github.com/paritytech/parity-common/blob/master/CONTRIBUTING.md#releasing-a-new-version

ordian commented 6 days ago

FWIW the alloy_primitives U256 doesn't support this conversion neither (although I don't know the exact reason).

Implementing From where the conversion isn't obvious is against the API guidelines. See the last point here. They even mention bytes to number conversion as an explicit example where to not implement it.

Agreed. This code in uint is very old and predates these guidelines probably.