GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add 'Suggestion' diagnostics · Pull Request #22204 · microsoft/TypeScript

Conversation

Deleted user (ghost) commented Feb 27, 2018

Copy link

Copy Markdown

Fixes #19392

Previously we had just syntax and semantic diagnostics. This adds a third layer of "info" diagnostics. They are only a part of services, so do not impact command line compilation.

Now when the editor sends us a "geterr" request, we will send 3 responses for "syntaxDiag", "semanticDiag", and "infoDiag". (See changes to updateErrorCheck.) All return a Diagnostic[].

For use from Visual Studio, this also adds a "infoDiagnosticsSync" command. (Andrew Casey (@amcasey) or would it be easier to just send it along with "semanticDiagnosticsSync"? Might impact performance as we add more info diagnostics though.)

For now there is only one info diagnostic, for a file that could be converted to commonJs. Moved the convertToEs6Module refactoring to a codefix.

CC Matt Bierner (@mjbvz)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Add 'info' diagnostics

f5ed9f3

ghost requested review from Andrew Casey (amcasey) and Sheetal Nandi (sheetalkamat)

February 27, 2018 18:56

Mohamed Hegazy (mhegazy) reviewed

Feb 27, 2018

View reviewed changes

Comment thread

src/compiler/types.ts Outdated

		export enum DiagnosticCategory {
		Warning,
		Error,
		Info,

Mohamed Hegazy (mhegazy) Feb 27, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I would call it Suggession. Info and Message are pretty much the same thing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Daniel Rosenwasser (DanielRosenwasser) Feb 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggession -> Suggestion

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) Feb 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

FWIW, Roslyn calls it Info. What is the difference between Info and Message? Is Message actually Telemetry?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) Feb 27, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Message is like description of a quick fix. user never sees that as a Diagnostic really.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) Feb 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This isn't quite what Roslyn calls Info, so I'm fine with either Info or Suggestion (or Hint, which appears to be the VS Code name).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Daniel Rosenwasser (DanielRosenwasser) Feb 28, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'd rather be consistent, but if we plan to have more-specific categories than Info in the future, Suggestion is fine with me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

Feb 27, 2018

View reviewed changes

Comment thread

src/server/protocol.ts Outdated


		/**
		* The category of the diagnostic message, e.g. "error" vs. "warning"
		* The category of the diagnostic message. This is the lower-case of a DiagnosticCategory member.

Mohamed Hegazy (mhegazy) Feb 27, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

the generated protocol.d.ts does not have DiagnosticCategory. so I would keep the comment in there.

One other thing also is we allow extensions to set it, so it is possible that there are other values here. so a union of string literals would be too restrictive for clients.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

Feb 27, 2018

View reviewed changes

Comment thread

src/server/session.ts Outdated

		if (diags) {
		const bakedDiags = diags.map((diag) => formatDiag(file, project, diag));
		this.event<protocol.DiagnosticEventBody>({ file, diagnostics: bakedDiags }, "syntaxDiag");
		// TODO: why do we check for diagnostics existence here, but in semanticCheck send unconditionally?

Mohamed Hegazy (mhegazy) Feb 27, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

we should report both unconditionally

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) Feb 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

FYI Arthur Ozga (@aozgaa)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

Feb 27, 2018

View reviewed changes

Comment thread

src/services/infoDiagnostics.ts Outdated

		@@ -0,0 +1,8 @@
		/* @internal */
		namespace ts {
		export function infoDiagnostics(sourceFile: SourceFile): Diagnostic[] {

Mohamed Hegazy (mhegazy) Feb 27, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

consider naming it compute*Diagnostics

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) commented Feb 27, 2018

Copy link

Copy Markdown

Contributor

Just a few comments about the name of the enum value. Andrew Casey (@amcasey) any comments?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mohamed Hegazy (mhegazy) approved these changes

Feb 27, 2018

View reviewed changes

Mohamed Hegazy (mhegazy) left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

With the enum name change

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) approved these changes

Feb 27, 2018

View reviewed changes

Comment thread

src/compiler/types.ts Outdated

		export enum DiagnosticCategory {
		Warning,
		Error,
		Info,

Andrew Casey (amcasey) Feb 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This isn't quite what Roslyn calls Info, so I'm fine with either Info or Suggestion (or Hint, which appears to be the VS Code name).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/server/protocol.ts Outdated

		arguments: InfoDiagnosticsSyncRequestArgs;
		}

		export interface InfoDiagnosticsSyncRequestArgs extends FileRequestArgs {

Andrew Casey (amcasey) Feb 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can/should interface declarations be shared across the various kinds of diagnostics?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/server/session.ts Outdated

		if (diags) {
		const bakedDiags = diags.map((diag) => formatDiag(file, project, diag));
		this.event<protocol.DiagnosticEventBody>({ file, diagnostics: bakedDiags }, "syntaxDiag");
		// TODO: why do we check for diagnostics existence here, but in semanticCheck send unconditionally?

Andrew Casey (amcasey) Feb 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

FYI Arthur Ozga (@aozgaa)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/services/codefixes/convertToEs6Module.ts

		const errorCodes = [Diagnostics.File_is_a_CommonJS_module_it_may_be_converted_to_an_ES6_module.code];
		registerCodeFix({
		errorCodes,
		getCodeActions(context) {

Andrew Casey (amcasey) Feb 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This seems much simpler. 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tests/cases/fourslash/javascriptModules22.ts Outdated

		verify.completionListContains("eggs");
		edit.insert("eggs;");
		verify.noErrors();
		verify.noErrors({ ignoreInfoDiagnostics: true });

Andrew Casey (amcasey) Feb 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Personally, I would expect noDiagnostics to mean "no diagnostics of any severity" and noErrors to mean "no diagnostics of error severity".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) commented Feb 28, 2018

Copy link

Copy Markdown

Member

I'm going to try hacking up my local VS build to confirm that I can consume this. I don't expect issues.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) commented Feb 28, 2018

Copy link

Copy Markdown

Member

I've created an internal PR that makes VS consume the new API. LGTM

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Code review

fb92a7e

ghost force-pushed the info branch from 65e8f56 to fb92a7e Compare

February 28, 2018 16:07

Deleted user (ghost) commented Feb 28, 2018

Copy link

Copy Markdown

Author

Andrew Casey (@amcasey) Mohamed Hegazy (@mhegazy) good to go? (Note that since name has changed to "suggestion", VS will need to change too.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) commented Feb 28, 2018

Copy link

Copy Markdown

Member

I'm fine with either "Info" or "Suggestion" and I've already confirmed that I can consume the new API from VS.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ghost merged commit fa4619c into master

Feb 28, 2018

ghost deleted the info branch

February 28, 2018 19:16

Mohamed Hegazy (mhegazy) changed the title ~~Add 'info' diagnostics~~ Add 'Suggestion' diagnostics

Feb 28, 2018

Matt Bierner (mjbvz) mentioned this pull request

Mar 9, 2018

Support for TypeScript's suggestion diagnostics microsoft/vscode#45378

Closed

ghost mentioned this pull request

Apr 9, 2018

Rename infoCheck to suggestionCheck #23286

Merged

Microsoft (microsoft) locked and limited conversation to collaborators

Jul 31, 2018

This pull request was closed.

Reviewers

Sheetal Nandi (sheetalkamat)

Awaiting requested review from sheetalkamat

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

codeFixes for things that aren't diagnostics

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Deleted user (ghost) commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) commented Feb 27, 2018

Uh oh!

Mohamed Hegazy (mhegazy) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) commented Feb 28, 2018

Uh oh!

Andrew Casey (amcasey) commented Feb 28, 2018

Uh oh!

Deleted user (ghost) commented Feb 28, 2018

Uh oh!

Andrew Casey (amcasey) commented Feb 28, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Daniel Rosenwasser (DanielRosenwasser) Feb 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.