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

Book tips and best practices #367

Merged

Conversation

hamiltop
Copy link
Contributor

Also update the book template and config. Lots of changes here, very open to feedback.

For now, just a few performance tips but eventually we'll want
to add other things.
# extern crate proptest_derive;
# extern crate proptest;
# use proptest_derive::Arbitrary;
# use proptest::prelude::*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what do all these pound signs do?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I take it from reading this later // doctests don't run under cfg(test), so we need to do this. that this hides this code in the docs but it's useful for compilation of the doc test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. This is a weird hack, but you got the main idea.

Pound signs hide these lines from the rendered docs, so we just show the bit that's useful for people to see.

And because doctests aren't compiled under cfg test, the test cases don't get defined and can't be called from main unless we also define them with cfg not test.

```rust,mdbook-runnable
use std::cell::RefCell;

# struct MyState {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

MyState isn't actually used in the test it looks like. In terms of communicating what's happening, i'm not sure the My prefix is that helpful either and I'm guessing if State has connect it's really ConnectionPool? what do you think about using that name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this now, I think this example is broken. I'll revisit.

# fn test_add() {
# let mut runner = TestRunner::default();
# runner.run(/* uhhm... */).unwrap();
# }
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's does adding a not test version of the same code do?

Copy link
Collaborator

Choose a reason for hiding this comment

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

so does this compile and run in the background make it appear to the user that the #[test] version ran where actually this did?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the actual doctest is the cfg not test fn. The test function is just for display purposes. There's probably a better way to DRY this up. I'll take another pass at it.

Copy link
Collaborator

@rex-remind101 rex-remind101 left a comment

Choose a reason for hiding this comment

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

just a few questions out of curiosity, this looks quite good to me

@hamiltop
Copy link
Contributor Author

hamiltop commented Sep 7, 2023

@rex-remind101 Took a second pass through and found a less hacky-hack to deal with the cfg test stuff. I also enabled a bunch more proptests.

One additional quirk: mdbook test cannot naively handle extern crates, so to run the tests you have to run it with mdbook test -L ../target/debug/deps

It might be worth exploring https://github.com/tfpk/mdbook-keeper/ (which solves many of these quirks).

Copy link
Collaborator

@rex-remind101 rex-remind101 left a comment

Choose a reason for hiding this comment

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

looks good 👍

@hamiltop hamiltop marked this pull request as ready for review September 9, 2023 00:25
@hamiltop
Copy link
Contributor Author

hamiltop commented Sep 9, 2023

Who can approve the workflow so CI can run?

Copy link
Member

@matthew-russo matthew-russo left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up and the new additions!

@matthew-russo matthew-russo merged commit fcccad0 into proptest-rs:master Sep 11, 2023
5 checks passed
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