-
Notifications
You must be signed in to change notification settings - Fork 163
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
Book tips and best practices #367
Conversation
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::*; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 {}; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(); | ||
# } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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
00fe3fa
to
6f6d54d
Compare
@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: It might be worth exploring https://github.com/tfpk/mdbook-keeper/ (which solves many of these quirks). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good 👍
Who can approve the workflow so CI can run? |
There was a problem hiding this 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!
Also update the book template and config. Lots of changes here, very open to feedback.