Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve docs crate wide #512

Merged
merged 3 commits into from
Nov 16, 2022
Merged

Conversation

tcharding
Copy link
Member

Read over and improve various parts of the crate documentation. Note this is an API breaking PR because it removes the public ONE_KEY type that we exposed when writing the docs for the secret module, exposing this type was, in my opinion, a mistake.

apoelstra
apoelstra previously approved these changes Nov 14, 2022
Copy link
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 0f9d295

@apoelstra
Copy link
Member

Needs rebase.

@apoelstra
Copy link
Member

Checked that we don't use ONE_KEY anywhere in the rust-bitcoin ecosystem. I kinda wonder why I had that constant.

The grammar on structs that implement `Context` using preallocated
memory is slightly incorrect. Attempt to improve the grammar.
The `alloc_only` module already has a docs guard on the "alloc" feature,
using an additional docs guard on the `SignOnly`, `VerifyOnly`, `All`
enums leads to a redundant feature combination "alloc" and "alloc or
std" - we really only require "alloc".
The `ONE_KEY` is only used in two rustdoc examples, as such it
unnecessarily pollutes the crate root namespace. We can use
`SecretKey::from_str()` with no loss of clarity and remove the
`ONE_KEY`.

While we are touching the import statements in `secret.rs` elect to
remove the hide (use of `#`) for import statements relating to this
library. Doing so gives devs all the information they need in one place
if they are using the examples to copy code. It is also in line with the
rest of the codebase.
Copy link
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 ec47198

@apoelstra apoelstra merged commit 4ff168b into rust-bitcoin:master Nov 16, 2022
@tcharding tcharding deleted the 11-10-more-docs branch November 17, 2022 05:03
@vkgnosis
Copy link

vkgnosis commented Jul 6, 2023

It would have been nice to document this breaking change in the changelog along with the fix of using from_str. Probably not worth doing now but I wanted to mention this for future changes.

@tcharding
Copy link
Member Author

I can add a changelog entry, though I don't understand the from_str comment because users can still do SecretKey(constants::ONE) in their own code if this const is needed.

@apoelstra
Copy link
Member

@tcharding they can't, becauuse the inner field of SecretKey is private, but even if they could, it would still be a breaking change to remove the old constant.

@vkgnosis
Copy link

vkgnosis commented Jul 7, 2023

Yes, exactly. The workaround is easy, it just took me some time to discover what happened to the constant. It would have taken less time if I had seen mention of it in the changelog for the version that removed it.

@tcharding
Copy link
Member Author

tcharding commented Jul 8, 2023

My bad, I'll try be more careful. Sorry for stealing your time @vkgnosis, thanks for reporting it.

@tcharding
Copy link
Member Author

Done in #622

@vkgnosis
Copy link

Thank you. You also saved me a bunch of time by working on this library. Much more than was taken by this small problem. I appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants