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

Support node ancesters via a configuration option #60

Open
4 tasks done
remcohaszing opened this issue Aug 22, 2023 · 3 comments
Open
4 tasks done

Support node ancesters via a configuration option #60

remcohaszing opened this issue Aug 22, 2023 · 3 comments
Labels
🤞 phase/open Post is being triaged manually

Comments

@remcohaszing
Copy link
Member

Initial checklist

Problem

You might also support ancestors: https://github.com/vfile/vfile-message#fields

Originally posted by @wooorm in #57 (review)

I looked into ancestors, but I think the information is not relevant here. It makes the messages very verbose. As an end user of the language server, the fact that everything is based on ASTs, is an implementation detail.

Originally posted by @remcohaszing in #57 (comment)

I’m not saying it should be a default. It isn’t in vfile-reporter either.

But I see if quite different. There are many reasons for errors to be thrown. I think users and the code that emits errors should decide whether the positional info and other info in the AST is an “implementation detail” or not.

Originally posted by @wooorm in #57 (comment)

Solution

Add option showAncestors. If this is true, the ancestry will be added to the LSP diagnostic message.

Alternatives

  1. showAncestors was chosen arbritrarily. The name could be different.
  2. Use DiagnosticRelatedInformation instead of appending to the message. This makes the trace clickable, but it renders differently.
@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Aug 22, 2023
@remcohaszing remcohaszing mentioned this issue Aug 22, 2023
5 tasks
@wooorm
Copy link
Member

wooorm commented Aug 22, 2023

What about verbose? https://github.com/vfile/vfile-reporter#options, unifiedjs/unified-engine@f44e365

IMO showing URLs here do makes sense, there’s more space here, and there’s links, so you don’t show an entire long ugly URL

@wooorm
Copy link
Member

wooorm commented Aug 22, 2023

DiagnosticRelatedInformation might be useful for cause? (which are now always shown in vfile-reporter: https://github.com/vfile/vfile-reporter/blob/f8f9655dbaa5f6b8d1d8208a9086363ffc865ce0/lib/index.js#L368-L370).

I’d appreciate an example maybe of how the “popups” look with different types of info and settings, maybe it gets crowded quickly?

@remcohaszing
Copy link
Member Author

One example of diagnostic related information is when you make a TypeScript type error in VSCode.

A TypeScript diagnostic pop-up with related information

In this screenshot, main.d.ts(115, 5): The expected type comes from property 'character' which is declared here on type 'Position' is a rendered diagnostic related information. I’m not really sure if diagnostic related information is the correct field for such a trace, but it has crossed my mind. I’m not really sure if it’s even ordered.

I like the traceLimit option from vfile-reporter. We could use that, and default to 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤞 phase/open Post is being triaged manually
Development

No branches or pull requests

2 participants