miette icon indicating copy to clipboard operation
miette copied to clipboard

Render inner diagnostics

Open TheNeikos opened this issue 2 years ago • 5 comments

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 ad9d0378ddd6dd4a6b9e650dd6aa62116da65910)

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?)

TheNeikos avatar May 06 '22 08:05 TheNeikos

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)

TheNeikos avatar May 07 '22 14:05 TheNeikos

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!

zkat avatar May 07 '22 16:05 zkat

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 https://github.com/zkat/miette/pull/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.

TheNeikos avatar May 07 '22 21:05 TheNeikos

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.

Porges avatar May 17 '22 23:05 Porges

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?

TheNeikos avatar Jun 01 '22 18:06 TheNeikos

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

zkat avatar May 17 '23 00:05 zkat

@zkat there we go!

TheNeikos avatar May 18 '23 12:05 TheNeikos

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

TheNeikos avatar May 18 '23 14:05 TheNeikos

Thanks all for getting this in! 🎉

Porges avatar May 18 '23 23:05 Porges