Skip to content

consensus_encoding: Go over docs with a fine toothcomb#5899

Merged
apoelstra merged 7 commits intorust-bitcoin:masterfrom
tcharding:push-rxoookttsktx
Mar 31, 2026
Merged

consensus_encoding: Go over docs with a fine toothcomb#5899
apoelstra merged 7 commits intorust-bitcoin:masterfrom
tcharding:push-rxoookttsktx

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Mar 25, 2026

Nick has this crate clean AF and ready to 1.0. I just went over the whole crate docs for any minute improvement.

Add an error submodule. This is a docs related change because it is explicitly done to improve the HTML rendering of the docs.

Copy link
Copy Markdown
Contributor

@nyonson nyonson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK db8d39b

@tcharding
Copy link
Copy Markdown
Member Author

Ooph had typo in commit log (title). PR is docs only if I removed the whitespace change. Electing to leave for Andrew to merge.

@apoelstra
Copy link
Copy Markdown
Member

This looks great. I went over the docs. I'm happy to merge this and then to just cut 1.0. But to be a bit pedantic:

  • The front-page docs list the decode_from free functions but don't mention that there are unbuffered variants, and don't mention the flush_to functions
  • The macros don't have examples, so when you click on them it's hard to tell how to invoke them
  • The list of top-level structs have one-line docs which sometimes start with A/An, sometimes with The, and sometimes with no article

I think we can defer these to a point release. And we know (I think) that we want to do a 1.1 eventually with hex support, once we've gotten the hex encoding stuff worked out properly.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Mar 30, 2026

I'm happy to merge this and then to just cut 1.0 But to be a bit pedantic:

Pedant away! I'd prefer to have a go at getting all your observations fixed. I can do it today. Also I rekon we want #5897 in before we do 1.0 so we don't have to do 1.1 immediately.

And we know (I think) that we want to do a 1.1 eventually with hex support, once we've gotten the hex encoding stuff worked out properly.

Oh I did not have this on my radar. What exactly are you referring to pls?

Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 76e6cb0; successfully ran local tests

`rust-bitcoin` project uses 100 column width for rustdoc. Now that
`consensus_encoding` is a hair width away from 1.0 update all the
rustdocs to use it.

However do not be dogmatic; if a doc is better with slightly
different colum width then do so.
This is a style change; fit in with the rest of the repo.
Mention all the free standing functions in the crate level docs.
Add an examples section to the rustdocs for the two public macros.
In order to make the HTML docs render more clearly it would be nice to
have all the errors separate. We recently introduced a policy for
doing this repo wide.

Add a public `error` submodule and re-export all errors at the crate
root.

Because of anomalies in how `cargo` renders docs for re-exports
between types and modules explicitly use `doc(inline)` and `no_inline`
so we get exactly the behaviour we desire.
@tcharding
Copy link
Copy Markdown
Member Author

Please note the final patch may seem controversial but is inline with recently introduced policy for error submodules.

@tcharding
Copy link
Copy Markdown
Member Author

fmt issues are in hashes not here.

Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 34281a5; successfully ran local tests

@apoelstra apoelstra merged commit 7cc30ea into rust-bitcoin:master Mar 31, 2026
27 of 29 checks passed
@tcharding
Copy link
Copy Markdown
Member Author

Not sure how we got green CI with the issues this introduces because it changes the API text files (in non-meaningful ways).

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Apr 1, 2026

Oof, it did have a red X for the API files, but it was way at the bottom where I didn't see it, and I ignored the "CI failed" because of the fmt job.

@tcharding
Copy link
Copy Markdown
Member Author

and I ignored the "CI failed" because of the fmt job.

I guessed this, I've already started ignoring CI too because of it. This red-always job may not be such a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-consensus_encoding PRs modifying the consensus-encoding crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants