Re-export UnprefixedHexError in the bitcoin crate root

[shinghim]

Re-export UnprefixedHexError in the bitcoin crate root

Fixes #2874

[coveralls]

Pull Request Test Coverage Report for Build 9618532760

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 83.12%

---

Totals
Change from base Build 9617142725:	0.0%
Covered Lines:	19549
Relevant Lines:	23519

---

💛 - Coveralls

[Kixunil]

ACK 535d702

[apoelstra]

Please commit the API changes.

[coveralls]

Pull Request Test Coverage Report for Build 9636043739

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 89 unchanged lines in 2 files lost coverage.
• Overall coverage remained the same at 83.12%

---

Files with Coverage Reduction	New Missed Lines	%
bitcoin/src/taproot/mod.rs	22	78.13%
bitcoin/src/bip32.rs	67	87.57%

Totals
Change from base Build 9617142725:	0.0%
Covered Lines:	19539
Relevant Lines:	23507

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 9636987268

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 89 unchanged lines in 2 files lost coverage.
• Overall coverage remained the same at 83.12%

---

Files with Coverage Reduction	New Missed Lines	%
bitcoin/src/taproot/mod.rs	22	78.13%
bitcoin/src/bip32.rs	67	87.57%

Totals
Change from base Build 9617142725:	0.0%
Covered Lines:	19539
Relevant Lines:	23507

---

💛 - Coveralls

[shinghim]

should i squash them all into one commit?

[tcharding]

There are a few problems with this PR:

• The api changes are incorrect because you have to use cargo public-api v0.35.0 (since CI: Pin cargo-public-api version #2893) but we never documented that - my bad.
• This doesn't close the issue because it leaves all the other error types in the units::parse module still unreachable.

#2874 is not a trivial issue to solve, it requires quite some thought (that is why I raised the issue instead of fixing the problem). For more context, for niche error types we typically expose the module so users can do foo::BarError.

So basically, the re-export of units and stuff in units is broken - the UnprefixedHexError is just one example of the broken-ness.

To move forward, and to close the issue, there needs to be some thought put into the units re-exports. FTR the re-exports everywhere are not perfect, we have not 100% nailed down the best way to re-export things.

[tcharding]

Ah bother, the check-api stuff is a mess. installation process is confusing.

EDIT: Resolved in #2900

[tcharding]

Your check-api problems stem from, I believe, the method used to install cargo-public-api. Try removing it and re-installing using cargo install --locked cargo-public-api@0.35.0.

See #2900 for context.

[shinghim]

Your check-api problems stem from, I believe, the method used to install cargo-public-api. Try removing it and re-installing using cargo install --locked cargo-public-api@0.35.0.

See #2900 for context.

I did some digging and realized this was the issue and ran just check-api, which resulted in the 3 lines being added for the API changes (how it currently looks). Previously, i had just done a cargo install cargo-public-api which resulted in the 1700+ line diff

[shinghim]

For more context, for niche error types we typically expose the module so users can do foo::BarError.

I've only started taking a look at the repo recently - do we know how niche the units::parse errors are? For more common error types, is there a desired way to export them?

[tcharding]

Ah I misspoke, I should not have said "niche", I should have said "for error useage we typically expose the module". I.e., we need a module so users can do parse::UnprefixedHexError. The question is should it be exposed at bitcoin's crate root eg, should users be able to do

pub use bitcoin::parse;

...
// The use `parse::UnprefixedHexError`

or should they have to go to units?

pub use bitcoin::units::parse;

(Remembering that we are missing a pub extern crates units also.)

[shinghim]

hmm i think that since that those error types aren't used only for the types in units (a couple examples of uses outside units here and here), it would make sense to expose it at the bitcoin crate root with pub use bitcoin::parse; This would also give us some flexibility if for whatever reason we move those errors out of units, we could just update the reexport block from

pub mod parse {
    /// Re-export everything from the [`units::parse`] module.
    pub use units::parse::ParseIntError;
    pub use units::parse::UnprefixedHexError;
}

to

pub mod parse {
    /// Re-export everything from the [`units::parse`] module.
    pub use some::new::path::ParseIntError;
    pub use some::new::path::UnprefixedHexError;
}

without affecting the end user. Would like to hear your thoughts to make sure I didn't miss anything

[tcharding]

I think you are right and this is the best approach.

I would improve your suggestion by writing it like this

As of today (this PR), put in bitcoin:

pub mod parse {
    //! Parsing utilities.

    /// Re-export everything from the [`units::parse`] module.
    #[doc(inline)]
    pub use units::parse::{ParseIntError, UnprefixedHexError}; // TODO: Add all public structs, enums, and functions
}

In the not-too-distant-future when we have primitives I think we should re-export at each level, so that each of these imports is possible and does the same thing

use bitcoin::parse;
use primitives::parse;
use units::parse;

So in primitives (primitives doesn't exist yet but its coming)

pub mod parse {
    //! Parsing utilities.

    /// Re-export everything from the [`units::parse`] module.
    #[doc(inline)]
    pub use units::parse::{ParseIntError, UnprefixedHexError}; // TODO: Add all public structs, enums, and functions
}

Then in bitcoin:

pub mod parse {
    //! Parsing utilities.

    /// Re-export everything from the [`primitives::parse`] module.
    #[doc(inline)]
    pub use primitives::parse::{ParseIntError, UnprefixedHexError}; // TODO: Add all public structs, enums, and functions
}

@Kixunil and @apoelstra should probably weigh in before you implement my ideas :)

[apoelstra]

Yeah, that looks good to me.

[tcharding]

Nice, we can move forward on this @shinghim. We are in the process of eliminating wildcard imports so just in case you get tempted please don't use pub use units::parse::* :)

[coveralls]

Pull Request Test Coverage Report for Build 9672691887

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 83.156%

---

Totals
Change from base Build 9667039141:	0.0%
Covered Lines:	19569
Relevant Lines:	23533

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 9672938948

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 83.156%

---

Totals
Change from base Build 9667039141:	0.0%
Covered Lines:	19569
Relevant Lines:	23533

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 9679309763

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 83.156%

---

Totals
Change from base Build 9667039141:	0.0%
Covered Lines:	19569
Relevant Lines:	23533

---

💛 - Coveralls

[Kixunil]

I think it makes sense. std/alloc/core do the same thing.

[Kixunil]

Looks good, unsure if we require splitting the API changes.

[coveralls]

Pull Request Test Coverage Report for Build 9684363160

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 83.094%

---

Totals
Change from base Build 9683507234:	0.0%
Covered Lines:	19508
Relevant Lines:	23477

---

💛 - Coveralls

[tcharding]

ACK 10440b3