Conversation
|
My initial reaction: I think this is too much magic right now and we will regret going down this path. Doing AST-rewriting into |
I think this is a relatively modest amount of magic for a huge ergonomics improvement. It is a well-defined rewrite between two public facing APIs that fails gracefully when it cannot be applied, and in exchange it gives you stable names for all your drawn values, which is most of the upside of the up front method that we lost with the inline method. I think it's also a precondition for good usability on #144, so I'm extremely keen on getting it in. |
|
To put it into context: I think this is less magic than we routinely perform in Hypothesis, and it gives us better ergonomics than the equivalent features in Hypothesis. |
253e769 to
27698f8
Compare
This introduces draw_named as the primitive for when we are actually printing, which provides much nicer output for values. It then makes the hegel::test macro automatically rewrite draw() to draw_named where it is safe to do so.
- Remove explicit 'a lifetime in is_tc_draw_binding (clippy::needless_lifetimes) - Move NOTE text outside ```no_run code fence in draw() doc comment - Allow clippy::let_and_return in closure test (needed for macro rewrite testing) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The candidate += 1 path in record_named_draw was only exercised by a TempRustProject test (separate process), so it didn't count for library coverage. This inline test directly exercises the skip logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
27698f8 to
8fbf6d5
Compare
|
I'm going through this now, but one thought at the top of my mind - I don't want |
Liam-DeVoe
left a comment
There was a problem hiding this comment.
LGMT! Excited to get this in.
I renamed draw_named to __draw_named, because I don't want to make this API public yet. Happy to debate that when you're back 🙂
Did a huge cleanup pass of the tests. They now assert stronger statements about the actual printed lines.
Fixed one bug - we previously had this test:
#[test]
fn test_macro_top_level_same_name_panics() {
let code = r#"
use hegel::generators as gs;
#[hegel::test(test_cases = 1)]
fn test_dup(tc: hegel::TestCase) {
let x = tc.draw(gs::booleans());
let x = tc.draw(gs::booleans());
}
"#;
TempRustProject::new()
.test_file("test_dup.rs", code)
.expect_failure(r#"draw_named.*"_x".*more than once"#)
.cargo_test(&["--test", "test_dup"]);
}which should clearly be passing instead with names x_1 and x_2.
This introduces draw_named as the primitive for when we are actually printing, which provides much nicer output for values.
It then makes the hegel::test macro automatically rewrite draw() to draw_named where it is safe to do so.
The result is that if you have a test that looks something like:
You will see output like:
If you were to write it like this without the intermediate variable:
You would see:
Users are able to name things directly if they want. It's a bit unergonomic but not awful. e.g.
is equivalent to the first example.