Skip to content

[minor] Adds some docs to the LSP interface #36740

Merged
orta merged 5 commits intomicrosoft:masterfrom
orta:jsdocs_ts_dts
Mar 13, 2020
Merged

[minor] Adds some docs to the LSP interface #36740
orta merged 5 commits intomicrosoft:masterfrom
orta:jsdocs_ts_dts

Conversation

@orta
Copy link
Copy Markdown
Contributor

@orta orta commented Feb 11, 2020

This is mainly to improve the understandability of using the LSP, it's not complete but it's movement.

Comment thread src/harness/client.ts Outdated
getProgramDiagnostics(): Diagnostic[] {
return this.getCompilerOptionsDiagnostics();
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the only thing which isn't JSDoc improvements, it fixed a TODO item I agreed with:

        // TODO: Rename this to getProgramDiagnostics to better indicate that these are any		         /**
         // diagnostics present for the program level, and not just 'options' diagnostics.		

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume this means “diagnostics not related to any particular source file”? My first thought when I saw getProgramDiagnostics was that it included all diagnostics for the program, including file-specific ones, since a Program is fundamentally a collection of files and options. If my assumption about what these diagnostics really are (which should be double-checked) is correct, I think getGlobalDiagnostics would have better conveyed the meaning to me. If this is going to be the canonical public API going forward though, I think it’d be good to get a consensus on the name beyond just the two of us (and whoever wrote that TODO).

Copy link
Copy Markdown
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

  1. Copy edits ✍️
  2. Let’s clarify getProgramDiagnostics and bikeshed the name with at least one or two others on the team 🚲🏚

Comment thread src/services/services.ts Outdated
Comment on lines +1455 to +1459
// @ts-ignore - 'getProgramDiagnostics' is declared but its value is never read - this is because it is a dupe of getCompilerOptionsDiagnostics
// so that the external interface makes more sense.
function getProgramDiagnostics() {
return getCompilerOptionsDiagnostics();
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don’t understand why this can’t just be deleted—removing it wouldn’t change the public API, right?

Comment thread src/harness/client.ts Outdated
getProgramDiagnostics(): Diagnostic[] {
return this.getCompilerOptionsDiagnostics();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume this means “diagnostics not related to any particular source file”? My first thought when I saw getProgramDiagnostics was that it included all diagnostics for the program, including file-specific ones, since a Program is fundamentally a collection of files and options. If my assumption about what these diagnostics really are (which should be double-checked) is correct, I think getGlobalDiagnostics would have better conveyed the meaning to me. If this is going to be the canonical public API going forward though, I think it’d be good to get a consensus on the name beyond just the two of us (and whoever wrote that TODO).

Comment thread src/services/types.ts Outdated
Comment thread src/services/types.ts
Comment thread src/services/types.ts
Comment thread src/services/types.ts Outdated
Comment thread src/services/types.ts Outdated
Comment thread src/services/types.ts Outdated
Comment thread src/services/types.ts

/**
* Gets semantic information about the identifier at a particular position in a
* file. Quick info is what you typically see when you hover in an editor.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

💯

Comment thread src/services/types.ts Outdated
Comment thread src/services/services.ts Outdated
orta and others added 2 commits March 13, 2020 12:02
@orta
Copy link
Copy Markdown
Contributor Author

orta commented Mar 13, 2020

I've pulled the getCompilerOptionsDiagnostics -> getProgramDiagnostics change, making this just a JSDoc change

@orta orta merged commit f1cc8e4 into microsoft:master Mar 13, 2020
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants