fixed-hash: remove rustc-hex feature

[koushiro]

merge #872 first

• remove rustc-hex feature (rustc-hex is only used for std::str::FromStr impl, so I don't think the feature is necessary)
• use const-hex instead of rustc-hex dependency (rustc-hex is no longer maintained, and const-hex has better performance)

BTW, I'm considering replacing both hex and rustc-hex in dependency tree with const-hex.

[koushiro]

@ordian @bkchr PTAL

[koushiro]

@ordian Is there anyone else who can review this PR?

[niklasad1]

why is const-hex not an optional dependency because rustc-hex was that before?

[koushiro]

rustc-hex is only used for std::str::FromStr impl, IMO, I don't think the feature is necessary.

if rustc-hex has a trait, like rustc_hex::Trait, and we need to impl rustc_hex::Trait for H256, then I agree that we should add rustc-hex as an optional feature.

another question: why is hex not an optional dependency of uint? 😕

[niklasad1]

I'm more concerned with a larger dependency tree for users that can't opt-out from it rather than a small impl inside this crate itself...

Lots of crates in this repo are very old and I don't think had a policy when to make it optional or not. Probably that's why we haven't been consistent

Anyway, I prefer to make const-hex an optional dependency such that users can opt-out from it but included in the default features.

[koushiro]

But the most users don't learn anything from the name of the const-hex feature, because it is only used in the implementation of std::str::FromStr/core::str::FromStr.
so why we add an optional feature for a core/std trait impl? unless the impl has multiple implementations based on different features.

[koushiro]

I care more about ease of use and consistency

• ease of use: why I need use an optional const-hex feature when I want to use core::str::FromStr trait of H256
• consistency: why other crates, likeuint, not using hex as an optional dependency

[koushiro]

To be honest, this kind of feature declaration is not a good way.
If a type (likeH256) has many core/std trait implementations that depend on other crates, do we need to add many features for each crate? obviously, this is not a good idea.

[niklasad1]

To rephrase what I mean if the feature is not part of the core functionality of the crate/type then it should be an optional feature IMO (especially if it needs extra dependencies)

I don't follow why you mean with H256 because it's re-exported from several crates such as fixed_hash, primitive_types and ethereum_types

For instance for ethereum-types it makes sense to have rustc-hex as hard dependency because it's required to be able to parse hex strings in the ethereum-world IIRC but it's not necessarily case for fixed-hash because it's depends how it used but in most use-cases it's probably useful to have.

So, if you can motivate why it's essential From::hex_str for these types then I'm fine to have it has hard dependency but we shouldn't use different libraries in uint and fixed_hash for those. If const_hex is the way to go we should use it in uint as well i.e, not both use hex and const_hex

[koushiro]

I don't follow why you mean with H256 because it's re-exported from several crates such as fixed_hash, primitive_types and ethereum_types

H256 is just a type as example.

If const_hex is the way to go we should use it in uint as well i.e, not both use hex and const_hex

see description:

BTW, I'm considering replacing both hex and rustc-hex in dependency tree with const-hex.

[koushiro]

OK, let me summarize:

1. Is core::str::FromStr one of the core functions of fixed-hash?
   IMO, it is, so I will choose hex as its str parsing format (it will rely on a hex parsing library, whether it is const-hex/hex/rustc-hex, I choose const-hex), of course, if it's other str format, we can consider using feature to distinguish them, but there is no such requirement now.

2. Why do I say that the existing feature declaration (like rustc-hex feature) is not a good idea?

   I think features should only be used for:

   • different implementations of the same function
   • optional function
   • support third-party libraries

From the existing design of fixed-hash, core::str::FromStr is its core function, and I think it should't support third-party libraries through features (keep fixed-hash pure and avoid the restrictions of the orphan rule?), so there are specific types declared in primitive-types and ethereum-types and a series of impls crates.

[niklasad1]

Okey, thanks for the explanation.

It's not necessarily requires to have a FromStr implementation to support parsing hex strings for fixed-hash but
as you say it's quite annoying to have a feature-flag for it and makes it harder for everyone.

So, I'm onboard to make this hex feature required for the hash types but in the future if you want change stuff it's a much better way to open an issue to explain why something is bad and why you want to change it.

It's a more likely that we reject PRs that make breaking changes.

[koushiro]

I don't want to submit PRs to a repo that doesn't give me timely feedback, so close it