Render inner diagnostics #170

TheNeikos · 2022-05-06T08:46:15Z

Previously #[diagnostic_source] did not correctly render the inner diagnostic. This PR fixes that.

There's a change here that can be considered controversial:

Only print newlines when there is an actual header (in ad9d037)

I am fine with removing it, but then the nested diagnostic source will be offset by one line and appear floating when it does not print a header.

PS: This PR also includes things from #169. I will rebase once that is merged (hopefully?)

zkat

I'm a little concerned about all the extra whitespace, and the potential complexity of displaying multiple errors in their entirety (as opposed to just the header, which I think was ideal).

Any time we make changes like these, it's very important to understand what the end user experience might be like, both for developers and error consumers, and I'm not sure this matches up with what I'd like to see.

At the same time, there's a chance this is exactly the right way to address the "Related" errors issue that's been kinda hard to figure out.

I'd love to see more examples and in-depth discussion of the intention behind this design.

tests/graphical.rs

zkat · 2022-05-06T15:58:25Z

src/handlers/graphical.rs

+                        inner_renderer.footer = None;
+                        inner_renderer.render_report(&mut inner, diag)?;
+
+                        writeln!(f, "{}", textwrap::fill(&inner, opts))?;


What is, ultimately, the desired user experience for this? Can you give some examples, and how this scales for more complex errors? I'm concerned that this will just create a lot of unintended noise, rather than help users focus on the error at hand.

The idea is that if a developer chooses to go into giving a complete error experience they do not have to go and implement their own printer and instead can re-use the existing infrastructure.

As a concrete example I have the following error tree possible:

User gives a configuration file with a variable amount of components and how to initialize them

Each component then gets loaded and can fail for various reasons, so that users can easily fix their errors all errors are collected and exposed as related.

Each of these errors are a Diagnostic so that users get complete information with source_codes etc

An example output of this:

Now, this screenshot is without this patch, but once added I will be able to attach more info to the inner error and thus have a clear hierarchy of what happened and how the user could fix it.

TheNeikos · 2022-05-07T14:21:57Z

I'm a little concerned about all the extra whitespace, and the potential complexity of displaying multiple errors in their entirety (as opposed to just the header, which I think was ideal).

I am not sure why miette would need to be aware of this? If a user wishes to not have it displayed, they can choose not to use the features that miette gives them. Since with an empty impl of Diagnostic it looks exactly how the Error output.

I'm not sure this matches up with what I'd like to see.

I absolutely understand that this is what I'd call a 'controversial' change, so I def want this to evolve to help everyone using miette. Could you elaborate a bit more on what parts you think do not fit? (Unless they are those you've already mentioned)

I'd love to come up with some more examples/combinations of how this could look like in some situations. (I've also gone one step further regarding the related questions in #171 but I'd say that is even more controversial and can wait :P)

zkat · 2022-05-07T16:22:55Z

If a user wishes to not have it displayed, they can choose not to use the features that miette gives them.

At scale, a dev does not necessarily have control over whether the sources they display are Diagnostics or not, and they might be surprised when they suddenly get a ton more output.

Could you elaborate a bit more on what parts you think do not fit? (Unless they are those you've already mentioned)

I just think we can do better than just rendering the entire diagnostic inline like this.

I'd love to come up with some more examples/combinations of how this could look like in some situations.

Ultimately, this is what I really want (for this and for #171). I'm not sure I like how they look right now, and I'd like to explore some more ideas of how things look before we move on to implementation. Just figure out the best end result, regardless of current implementation, and work backwards from there. Heck, we can even change the general structure of error printing right now if it makes sense!

TheNeikos · 2022-05-07T21:51:35Z

At scale, a dev does not necessarily have control over whether the sources they display are Diagnostics or not, and they might be surprised when they suddenly get a ton more output.

Hm, I'd agree if source and diagnostic_source would be the same annotation, but if a dev puts diagnostic_source into their code then I do believe they should expect it to be printed as a diagnostic! (Otherwise, they could just use it as an Error & source)

I just think we can do better than just rendering the entire diagnostic inline like this.

Alright! I am not exactly sure what the alternatives are haha, but I'd love to discuss it.

Ultimately, this is what I really want (for this and for #171). I'm not sure I like how they look right now, and I'd like to explore some more ideas of how things look before we move on to implementation. Just figure out the best end result, regardless of current implementation, and work backwards from there. Heck, we can even change the general structure of error printing right now if it makes sense!

Sounds good to me, I'll be able to get back to it probably next week, so I'll try to hash out some nice error structures then, so we can compare/discuss.

Porges · 2022-05-17T23:13:04Z

if a dev puts diagnostic_source into their code then I do believe they should expect it to be printed as a diagnostic!

Yes, definitely agree here. It’s opt-in and I would expect (as seen in #172) the diagnostics to be included in this case.

TheNeikos · 2022-06-01T18:53:38Z

So, I've been thinking about this, and I would suggest the following path forward:

Create a recursive struct of all the information the errors contain (or don't contain)

struct ErrorDescription {
  error: String,
  causes: Vec<ErrorInformation>,
  related: Vec<ErrorInformation>,
  source_code: ...
}

Make the reporter accept that ErrorDescription and print its respective representation
Now that it is hopefully easier to refactor printing, discuss with more datapoints!

This would also make it easier for other crates to replace the reporter since they don't have to gather the information in the first place and simply use the given information.

What do you think?

zkat · 2023-05-17T00:55:48Z

Ok I think we can merge this. Do you mind rebasing/resolving conflicts?

Previously diagnostics just had their StdError Display implementation called. This now uses a GraphicalReport if wished.

TheNeikos · 2023-05-18T12:39:44Z

@zkat there we go!

zkat

One last thing: do you mind writing a quick test to show what these nested diagnostics actually look like?

Signed-off-by: Marcel Müller <neikos@neikos.email>

TheNeikos · 2023-05-18T14:22:41Z

One last thing: do you mind writing a quick test to show what these nested diagnostics actually look like?

Duh, totally forgot that one haha

It prints them indented as before, but simply prints them as a whole rather than just their debug info

Signed-off-by: Marcel Müller <neikos@neikos.email>

TheNeikos · 2023-05-18T15:24:58Z

tests/test_diagnostic_source_macro.rs

+    let expected = r#"  × A nested error happened
+  ├─▶   × TestError
+  │   
+  ╰─▶   × A complex error happened
+         ╭────
+       1 │ This is another error
+         ·    ──┬─
+         ·      ╰── here
+         ╰────
+        help: You should fix this
+
+   ╭────
+ 1 │ right here
+   ·       ──┬─
+   ·         ╰── here
+   ╰────
+
+  Yooo, a footer
+"#;


This is the crux of the change IMO. You can see that nested diagnostics actually get printed out as they are intended to be IMO

TheNeikos · 2023-05-18T15:25:51Z

tests/test_diagnostic_source_macro.rs

+   ╭────
+ 1 │ right here
+   ·       ──┬─
+   ·         ╰── here
+   ╰────


Currently causes come before snippets. This makes it look somewhat awkward. But I am not sure that this should be changed (if at all?) in this PR.

Porges · 2023-05-18T23:14:08Z

Thanks all for getting this in! 🎉

TheNeikos force-pushed the feature/render_inner_diagnostics branch 2 times, most recently from cb2c4a9 to 2b9b6af Compare May 6, 2022 09:56

zkat reviewed May 6, 2022

View reviewed changes

TheNeikos mentioned this pull request May 16, 2022

Wrapping other Errors without losing diagnostics #172

Closed

azriel91 mentioned this pull request Oct 23, 2022

User friendly output on the command line - second cut azriel91/peace#41

Closed

5 tasks

zkat force-pushed the main branch 3 times, most recently from 92a28b7 to b045321 Compare April 1, 2023 00:08

TheNeikos added 2 commits May 18, 2023 14:39

Add test verifying diagnostic

ce4d9c6

Render inner diagnostic with a renderer

66af796

Previously diagnostics just had their StdError Display implementation called. This now uses a GraphicalReport if wished.

TheNeikos force-pushed the feature/render_inner_diagnostics branch from 2b9b6af to a1c78f8 Compare May 18, 2023 12:39

TheNeikos force-pushed the feature/render_inner_diagnostics branch from a1c78f8 to 5b1d52a Compare May 18, 2023 12:52

Only print a newline sep. when there is a header

4791251

TheNeikos force-pushed the feature/render_inner_diagnostics branch from 5b1d52a to 4791251 Compare May 18, 2023 13:13

zkat reviewed May 18, 2023

View reviewed changes

Add test for diagnostic_source printing

1c53b57

Signed-off-by: Marcel Müller <neikos@neikos.email>

Do not print errors recursively

48f5dab

Signed-off-by: Marcel Müller <neikos@neikos.email>

TheNeikos commented May 18, 2023

View reviewed changes

zkat approved these changes May 18, 2023

View reviewed changes

zkat merged commit aefe323 into zkat:main May 18, 2023

TheNeikos deleted the feature/render_inner_diagnostics branch July 21, 2023 06:39

LMH01 mentioned this pull request Aug 21, 2023

Wrapping errors looses diagnostic information #286

Open

TheLostLambda mentioned this pull request Jan 27, 2024

fix(graphical): render cause chains for inner errors #330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render inner diagnostics #170

Render inner diagnostics #170

TheNeikos commented May 6, 2022 •

edited

Loading

zkat left a comment

zkat May 6, 2022

TheNeikos May 7, 2022

TheNeikos commented May 7, 2022

zkat commented May 7, 2022

TheNeikos commented May 7, 2022 •

edited

Loading

Porges commented May 17, 2022

TheNeikos commented Jun 1, 2022

zkat commented May 17, 2023

TheNeikos commented May 18, 2023

zkat left a comment

TheNeikos commented May 18, 2023

TheNeikos May 18, 2023

TheNeikos May 18, 2023

Porges commented May 18, 2023

Render inner diagnostics #170

Render inner diagnostics #170

Conversation

TheNeikos commented May 6, 2022 • edited Loading

zkat left a comment

Choose a reason for hiding this comment

zkat May 6, 2022

Choose a reason for hiding this comment

TheNeikos May 7, 2022

Choose a reason for hiding this comment

TheNeikos commented May 7, 2022

zkat commented May 7, 2022

TheNeikos commented May 7, 2022 • edited Loading

Porges commented May 17, 2022

TheNeikos commented Jun 1, 2022

zkat commented May 17, 2023

TheNeikos commented May 18, 2023

zkat left a comment

Choose a reason for hiding this comment

TheNeikos commented May 18, 2023

TheNeikos May 18, 2023

Choose a reason for hiding this comment

TheNeikos May 18, 2023

Choose a reason for hiding this comment

Porges commented May 18, 2023

TheNeikos commented May 6, 2022 •

edited

Loading

TheNeikos commented May 7, 2022 •

edited

Loading