Open eupn opened 2 years ago
Thanks for working on #65. I'll test out this branch as soon as I can and report back here.
I found the following problems with expand_fail
:
.expanded
file, the one created automatically is empty. I would have expected it to contain the error message from the macro that panicked..expanded
file, I always just get a <file path> - different!
error showing me the contents of the .expanded
file but nothing else. expand_fail
always seems to fail, even when my expected error is identical to the actual error.@Emoun can you show me the code for failing macro? I'll add this macro as an example/test if it's small enough. Does manually calling cargo expand
on this macro produce an error (with a non-zero exit code)?
I'm trying things on duplicate
, but the specific call I'm trying is:
#[duplicate::duplicate(refs(T); [& T]; [T])]
fn from(x: refs(Bits<1, false>)) -> bool {
x.value == 1
}
Expanding this will panic in duplicate
because of wrong use.
You can try it out on the branch i'm using. The specific call is at tests/default_features/test/parameters_not_encapsulated.rs
and the use of macrotest
is at tests/default_features/mod.rs:46
I tried running cargo expand
on a different project that used duplicate
, where I added the wrong code, and I can see that cargo expand does return 0 as an exit code, even though it also shows that the macro panicked.
Btw, I have cargo-expand v1.0.9
@Emoun, thank you! I'll explore this issue further.
@Emoun Indeed, cargo outputs expansion errors (such as proc macro panics) into STDERR while returning zero exit code. I've made some changes to handle this, could you check again?
Potentially will close #67.
I'll have a look at it this weekend and report back.
I've given the update a try and, in short, it seems for me to work well enough to be usable for my use cases.
I found one issue that I think is critical, though I specifically don't use macrotest
in this way. As such, it is not blocking to me specifically:
expand_fail
: When I have a macro that succeeds, but I expect it to fail, and there is no .expanded
file. A new .expanded
file is created, with the expansion result, and success is reported. This is problematic since if you run the tests again, you will again get a success (since the contents of the .expanded
match the output of the macro) however, the macro is not panicking, as it should when using expand_fail
. This can trip-up anyone not paying attention to what the contents of the generated .expanded
file are.Below are some of the irregularities I have found. While I think they should be addressed, they are not blocking for my use cases and mostly affect usability.
expand_fail
: When I have a macro that succeeds, but I expect it to fail, the error message I get gives me the difference between my expected error message, and the actual expansion output (i.e. a <file path> - Different!
error). I would instead expect to get a message telling me the macro was expected to panic but didn't. E.g. <file path> - Didn't Panic!
. I'm ambivalent to whether this could be followed by the macros actual expansion output.expand
: When run on a macro that panics, it correctly reports an error but the report doesn't include the initial <file path> - <error type>!
message signifying what test failed. I would prefer if each panic report starts with (e.g.) <file path> - Unexpected Panic!
followed by the macro's own error message. expand_without_refresh
/expand_without_refresh
: When run without any .expanded
file being present, correctly throws an error but there is no message. I would expect a message like <file path> - Missing '.expanded.rs' file!
.@eupn, do you have an ETA on when this might get merged and an eventual new release? This PR and the changes currently in master would be really nice to get into a new release.
Thank you for all your hard work on this nice crate.
@Emoun thanks for kind words! I'll try my best to publish a new release next week.
@eupn, sorry to bother again, but I'm still very much interested in this PR getting merged/released even without the last few fixes I highlighted above. I hope you can find some time to look into it.
Closes #65.
Please check with this branch, @Emoun. Should you get no issues, I'll merge this and release a new version.