Add examples to `key` module #376

tcharding · 2022-01-13T08:51:11Z

This PR is an initial attempt to more thoroughly test our public API.

Add examples to various types/methods/functions in the key module.

I'm not entirely sure when is enough, do we want an example on every single public method, function, and type or is this overkill. In this PR I tried to find a balance by doing ever method/function that took an argument that is a custom type from this lib. I think this should be extended to include return values too though ...

Thanks to @thomaseizinger for the idea!

First 2 patches are docs improvements to lib.rs.

src/key.rs

elichai · 2022-01-13T10:05:56Z

src/key.rs

+    /// # #[cfg(feature="rand")] {
+    /// use secp256k1::{rand, SecretKey};


rand isn't really used here

Oh cheers, I've not worked out how to lint the examples yet.

I haven't tried it but I'd expect --all-targets on cargo clippy to include rustdoc.

I only learned during this PR that -all-targets does not include docs (I have my editor configured to run clippy with --all-targets, thanks Lucas :)

src/key.rs

elichai · 2022-01-13T10:14:14Z

src/key.rs

+    /// let mut public_key = key_pair.public_key();
+    /// let original = public_key;
+    /// let parity = public_key.tweak_add_assign(&secp, &tweak).expect("Tweak error");
+    /// assert!(original.tweak_add_check(&secp, &public_key, parity, tweak));


Too bad we don't have access to a hash function to showcase this properly :(

What did you have in mind? We can access bitcoin_hashes here.

elichai · 2022-01-13T10:14:51Z

Sorry for the long review, it's easy to bikeshed over documentation

tcharding · 2022-01-13T21:39:31Z

Sorry for the long review, it's easy to bikeshed over documentation

No apology needed, I appreciate the comments. Documentation is definitely a funny thing, its easy to agree we need it not so easy to agree how it should be done. That's why I tried to stick meticulously to Rust conventions. (I've not written that much thorough Rust documentation in the past though so I'm still learning.)

tcharding · 2022-01-13T21:48:15Z

@elichai I fixed all the 'should have been found by the linter' issues and squashed them into the original commit (4fc759a Add examples to types and methods in key module). The OsRng suggestion is done as a separate patch along with changing how rand::thread_rng is called, please say if you disagree with that patch. Thanks.

thomaseizinger

Nice work.

thomaseizinger · 2022-01-13T23:22:00Z

src/key.rs

+/// # #[cfg(feature="rand")] {
+/// use secp256k1::{rand, Secp256k1, SecretKey};
+///
+/// let secp = Secp256k1::new();
+/// let secret_key = SecretKey::new(&mut rand::thread_rng());
+/// # }


Nice idea with the scope. Untested idea that might also work (note the #!):

Suggested change

/// # #[cfg(feature="rand")] {

/// use secp256k1::{rand, Secp256k1, SecretKey};

///

/// let secp = Secp256k1::new();

/// let secret_key = SecretKey::new(&mut rand::thread_rng());

/// # }

/// # #![cfg(feature="rand")]

/// use secp256k1::{rand, Secp256k1, SecretKey};

///

/// let secp = Secp256k1::new();

/// let secret_key = SecretKey::new(&mut rand::thread_rng());

This doesn't work with Rust 1.29 unfortunately. I think, IIRC, it starts working at 1.34. I'll add a TODO to remove them after bump of MSRV.

Fail! I did not 'note the !#' even when you told me too. It is hard to get good help :)
Updating this PR to include this suggestion.

Bother, and I thought the saying was 'if it builds ship it' ... turns out the !# suggestion builds just fine but doesn't run.

Oh that is weird. Should go back to testing my suggestions before making them!

Not at all, open suggestions lead to leaning stuff. And since they are cheaper to make than tested suggestions it leads to more information being shared, IMO.

src/key.rs

tcharding · 2022-01-17T21:53:32Z

After re-visiting this I've squashed all the suggested changes into the one commit 7e3af61 Add examples to types and methods in key module.

Improve method docs by doing: - Remove 'batch' comment - Remove mention of required features, docs already show this - Use links to types as well as ticks

Improve the main docs by doing: - Remove unneeded `self` from use statement - Add code ticks to `bitcoin_hashes`

Use 'standard' stlye, standard is defined as - No markdown heading - Full sentence (capital first letter and full stop) - Trailing empty comment line

Done in an effort to better test our public API. Add tests in the `Examples` section as is idiomatic in the Rust ecosystem. Make other minor improvements to any rusdocs we touch: - Use full stops - Use 100 character column width - Use plural third person tense - Use plural for section headings

We recently patched much of the docs in the `key` module, lets attempt to attain perfection. Improve docs by doing: - Use full stops - Use 100 character column width - Use plural third person tense - Use plural for section headings - Fix any grammar mistakes - Use code ticks and links as appropriate

apoelstra

ACK aa828f0

This is amazing, thanks!

tcharding · 2022-01-24T20:48:04Z

Cheers man!

aa51638 update changelog for 0.22.0 (Andrew Poelstra) d06dd20 update fuzzdummy API to match normal API (Andrew Poelstra) f3d48a2 update "should terminate abnormally" test to trigger a different ARG_CHECK (Andrew Poelstra) 8294ea3 secp256k1-sys: update upstream library (Andrew Poelstra) 2932179 secp256k1-sys: update secp256k1.h.patch (Andrew Poelstra) Pull request description: Should wait on merging until we get a minor release out with #382 and #376. May also want to bundle #380 with this? ACKs for top commit: real-or-random: ACK aa51638 I can't judge if the feature set is meaningful but this release PR is fine Tree-SHA512: e7f48b402378e280a034127f2de58d3127e04303a114f07f294fa3d00c0a083ae0d43375a8a74d226b13ea45fb3fde07d8450790e602bbf9581adc5fd8bc7d29

@thomaseizinger

aa828f01a5727f0b056975cdbcaedbb24efb78bd Improve documentation in the key module (Tobin Harding) 9e46d6f1220c66b095fa061017bf2596e270328f Add examples to types and methods in key module (Tobin Harding) a7f3d9bcfde1719966adf2d8abc3b2a6dd98d591 Improve key module docs (Tobin Harding) 6d23614467e8f51ce7a1b4defdd777f5320f6c4c Improve lib.rs rustdocs (Tobin Harding) 4c4268f1adde945acf1eb247ad8b2a235134749c Improve docs on method generate_keypair (Tobin Harding) Pull request description: This PR is an initial attempt to more thoroughly test our public API. Add examples to various types/methods/functions in the key module. I'm not entirely sure when is enough, do we want an example on every single public method, function, and type or is this overkill. In this PR I tried to find a balance by doing ever method/function that took an argument that is a custom type from this lib. I think this should be extended to include return values too though ... Thanks to @thomaseizinger for the idea! First 2 patches are docs improvements to `lib.rs`. ACKs for top commit: apoelstra: ACK aa828f01a5727f0b056975cdbcaedbb24efb78bd Tree-SHA512: 9383ad263469f98ce7e988d47edc1482a09a0ce82f43d3991bd80aabdf621430f4a3c86be4debf33232dcb1d60d3e81f2c6d930ea7de7aa0e34b037accd7bc98

@thomaseizinger

aa828f01a5727f0b056975cdbcaedbb24efb78bd Improve documentation in the key module (Tobin Harding) 9e46d6f1220c66b095fa061017bf2596e270328f Add examples to types and methods in key module (Tobin Harding) a7f3d9bcfde1719966adf2d8abc3b2a6dd98d591 Improve key module docs (Tobin Harding) 6d23614467e8f51ce7a1b4defdd777f5320f6c4c Improve lib.rs rustdocs (Tobin Harding) 4c4268f1adde945acf1eb247ad8b2a235134749c Improve docs on method generate_keypair (Tobin Harding) Pull request description: This PR is an initial attempt to more thoroughly test our public API. Add examples to various types/methods/functions in the key module. I'm not entirely sure when is enough, do we want an example on every single public method, function, and type or is this overkill. In this PR I tried to find a balance by doing ever method/function that took an argument that is a custom type from this lib. I think this should be extended to include return values too though ... Thanks to @thomaseizinger for the idea! First 2 patches are docs improvements to `lib.rs`. ACKs for top commit: apoelstra: ACK aa828f01a5727f0b056975cdbcaedbb24efb78bd Tree-SHA512: 9383ad263469f98ce7e988d47edc1482a09a0ce82f43d3991bd80aabdf621430f4a3c86be4debf33232dcb1d60d3e81f2c6d930ea7de7aa0e34b037accd7bc98

tcharding marked this pull request as draft January 13, 2022 08:56

tcharding force-pushed the add-examples-key branch from 7c6de93 to f003b08 Compare January 13, 2022 09:08

tcharding marked this pull request as ready for review January 13, 2022 09:08

elichai reviewed Jan 13, 2022

View reviewed changes

tcharding force-pushed the add-examples-key branch from f003b08 to 6d3919a Compare January 13, 2022 21:46

tcharding force-pushed the add-examples-key branch from 6d3919a to 5e660fe Compare January 13, 2022 21:53

thomaseizinger reviewed Jan 13, 2022

View reviewed changes

tcharding mentioned this pull request Jan 14, 2022

Add integration tests #371

Closed

tcharding force-pushed the add-examples-key branch from e81c6ef to 4a40a6a Compare January 17, 2022 21:52

tcharding force-pushed the add-examples-key branch from 4a40a6a to 694e46f Compare January 17, 2022 22:01

tcharding added 5 commits January 24, 2022 14:43

Improve docs on method generate_keypair

4c4268f

Improve method docs by doing: - Remove 'batch' comment - Remove mention of required features, docs already show this - Use links to types as well as ticks

Improve lib.rs rustdocs

6d23614

Improve the main docs by doing: - Remove unneeded `self` from use statement - Add code ticks to `bitcoin_hashes`

Improve key module docs

a7f3d9b

Use 'standard' stlye, standard is defined as - No markdown heading - Full sentence (capital first letter and full stop) - Trailing empty comment line

tcharding force-pushed the add-examples-key branch from 694e46f to aa828f0 Compare January 24, 2022 03:48

apoelstra approved these changes Jan 24, 2022

View reviewed changes

apoelstra mentioned this pull request Jan 24, 2022

update libsecp to latest ersion; bump major version number #384

Merged

apoelstra merged commit f7d637e into rust-bitcoin:master Jan 25, 2022

tcharding deleted the add-examples-key branch January 26, 2022 00:23

		/// # #[cfg(feature="rand")] {
		/// use secp256k1::{rand, SecretKey};

Uh oh!

Add examples to key module #376

Add examples to key module #376

Uh oh!

Conversation

tcharding commented Jan 13, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elichai commented Jan 13, 2022

Uh oh!

tcharding commented Jan 13, 2022

Uh oh!

tcharding commented Jan 13, 2022

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcharding commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Jan 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add examples to `key` module #376

Add examples to `key` module #376

tcharding commented Jan 17, 2022 •

edited

Loading