Skip to content

derive: make error messages slightly more readable, add manually-driven test #133

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

Merged
merged 1 commit into from
Apr 14, 2025

Conversation

BenWiederhake
Copy link
Contributor

@BenWiederhake BenWiederhake commented Apr 13, 2025

Closes a bit of #129.

Using a fully-fledged compile-error testsuite is a bit overkill, but we still want to make sure that the derive crate generates reasonable error messages. That's what this "example" is for. In the following, there are blocks of lines, one marked as POSITIVE and multiple lines marked as NEGATIVE. The committed version of this file should only contain POSITIVE. In order to run a test, comment out the POSITIVE line, and use a NEGATIVE line instead, and manually check whether you see a reasonable error message – ideally the error message indicated by the comment. One way to do this is:

$ cargo build --example test_compile_errors_manually

Example output with the type that I did in #129:

$ cargo build --example test_compile_errors_manually
   Compiling uutils-args v0.1.0 (/home/user/workspace/uutils-args)
error: proc-macro derive panicked
  --> examples/test_compile_errors_manually.rs:24:10
   |
24 | #[derive(Arguments)]
   |          ^^^^^^^^^
   |
   = help: message: expected '=' after '[' in flag pattern

It's not a particularly nice error message, but it's more readable than the current called `Option::unwrap()` on a `None` value.

@BenWiederhake BenWiederhake force-pushed the dev-derive-panic-no-unwrap branch from 9919060 to b2e78d3 Compare April 13, 2025 18:59
@BenWiederhake
Copy link
Contributor Author

Changes since last push: cargo fmt

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

I think this is a clear improvement, but maybe not enough to close the issue for two reasons.

The first is that I think a proper compile-test framework sounds pretty good for this crate, because it is so focused on derive-based functionality.

The second is that we would ideally use the spans from syn to generate error messages, so that the errors don't just get reported on the #[derive(Arguments)] but on the parts that are failing.

@BenWiederhake
Copy link
Contributor Author

Ah, well, then I'll add compile-fail-tests in the next PR :)

@tertsdiepraam tertsdiepraam merged commit fb6209a into uutils:main Apr 14, 2025
@tertsdiepraam
Copy link
Member

I unlinked the issue, but this is of course merge-worthy!

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.

2 participants