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

Add child diagnostics #225

Merged
merged 1 commit into from Mar 9, 2023
Merged

Add child diagnostics #225

merged 1 commit into from Mar 9, 2023

Conversation

TedDriggs
Copy link
Owner

@TedDriggs TedDriggs commented Mar 8, 2023

Fixes #224

Open Items

  • Update release notes
  • Add unit tests (details)
  • Update CI to run diagnostics unit tests

Copy link

@AlisCode AlisCode left a comment

Choose a reason for hiding this comment

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

This seems to do the work, good job! I've put some feedback, feel free to take it or not :)

core/src/error/child.rs Show resolved Hide resolved
core/src/error/child.rs Show resolved Hide resolved
core/src/error/mod.rs Outdated Show resolved Hide resolved
core/src/error/mod.rs Show resolved Hide resolved
@TedDriggs
Copy link
Owner Author

@AlisCode thanks for the great review so far! The area that I would most appreciate help with on this is coming up with some unit tests; these would serve as a sanity check for the functionality and catch ergonomic issues like the one about being able to pass string literals without needing &. The main challenges are:

  • ChildDiagnostic::append_to cannot be called from the unit test, or it will panic. I've filed Convert from proc_macro2::Span to proc_macro::Span without panic dtolnay/proc-macro2#365 to see if that can be addressed
  • Setting up CI to make sure that the test can run without failing due to a bad nightly build. I think your idea of pinning a specific nightly build for the diagnostics unit tests makes sense, so the GitHub workflows will need to be updated to do that.

@AlisCode
Copy link

AlisCode commented Mar 9, 2023

I'll see what I can do for the unit-test coverage of this, although I don't immediately see a good solution to the panic.
I'll try to update the GH workflow to pin the version of nightly like I suggested, and cross my fingers I don't break anything doing so :)

Thanks for the reactivity and the good work !

@TedDriggs
Copy link
Owner Author

@AlisCode Rather than always pinning nightly, I'd suggest adding a specific new job that tests diagnostics with that feature enabled and the nightly version pinned. That'll avoid changing the current test slate which runs against the latest nightly.

I think the best solution for the panic issue is to manually test append_to now and then just hope for the best going forward 😅 The unit tests can focus on making sure the ergonomics of adding these messages to an error work well.

Thanks for the reactivity and the good work !

Thanks for the great feature request!

When using the `diagnostics` feature, crate consumers can add custom
error, warning, note, and help messages to `Error` instances and have
those appear in the compiler's output.

Fixes #224
@TedDriggs TedDriggs changed the title WIP: Add child diagnostics Add child diagnostics Mar 9, 2023
@TedDriggs
Copy link
Owner Author

@AlisCode the doc tests I added seem to do a good enough job of making sure the happy-path compiles, so I'm going to declare this "good enough for a feature-flagged feature" and merge.

@TedDriggs TedDriggs merged commit 92abd90 into master Mar 9, 2023
@TedDriggs TedDriggs deleted the child-diagnostics branch March 9, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for custom diagnostics in Error
2 participants