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

feat(Diagnostics): Add proposed DiagnosticTag.Deprecated enum member #77760

Merged
merged 17 commits into from Jul 26, 2019

Conversation

@kamranayub
Copy link
Contributor

commented Jul 22, 2019

Closes #56694
Relates to #50972

Changes

  • Add proposed DiagnosticTag.Deprecated member
  • Add MarkerTag.Deprecated

TODO

@msftclas

This comment has been minimized.

Copy link

commented Jul 22, 2019

CLA assistant check
All CLA requirements met.

@kamranayub

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

@sandy081 @jrieken I'm still trying to understand the different aspects of this. I added MarkerTag like was done previously for Unnecessary but now that I re-read the proposal for #50972 I'm thinking that isn't needed because this won't be a marker (with a squiggly); it will somehow get tied to CompletionItem.deprecated or vice-versa. Basically somehow this will need to hint to style with a strike-through.

Am I understanding that correctly? If so, I can revert those two changes. Plus, limiting to just adding the new proposed enum member will be closer to the exact purpose of #56694

@jrieken

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

@kamranayub both requests, #56694 and #50972, are independent and need separate solutions. Having DiagnosticsTag.Deprecated will allow extensions to render certain diagnostics "special", e.g a common rendering of deprecated is a strikeout (in addition to the severity based rendering). Compare this to the faded rendering of unused/unnecessary code.

@jrieken jrieken added this to the July 2019 milestone Jul 23, 2019

@kamranayub

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Thanks for clarifying. I will revert the two Marker changes.

kamranayub added some commits Jul 23, 2019

@jrieken

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

I will revert the two Marker changes.

Not sure why you did that. Your change is now exposing a new enum member but it's not being used anywhere, e.g. it's not being translated into our "internal" API which is MarkerTag and it's not being read/used to apply any rendering tweaks

@kamranayub

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Ah, I assumed MarkerTag was specifically only for being able to render squigglies based on the usage I saw for MarkerTag.Unnecessary in 8bb27cd#diff-489874091c7caf27cd2ed52e59c41147

@jrieken jrieken removed this from the July 2019 milestone Jul 23, 2019

@kamranayub

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@jrieken Brought those changes back. Is the next step to try and perform some rendering using this new API or just add a test case for mapping the DiagnosticTag to MarkerTag?

@jrieken

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

perform some rendering using this new API or just ad

👍 the whole thing

@kamranayub

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Making progress.

Rendering CompletionItem.deprecated

image

Rendering MarkerTag.Deprecated

image

Both examples above are NOT actually implemented with the language provider, they are just hardcoded to include deprecated tag or set CompletionItem.deprecated so I could see the styling work.

diff --git a/extensions/typescript-language-features/src/features/completions.ts b/extensions/typescript-language-features/src/features/completions.ts
index fdfabd4365..89e80b8319 100644
--- a/extensions/typescript-language-features/src/features/completions.ts
+++ b/extensions/typescript-language-features/src/features/completions.ts
@@ -50,6 +50,9 @@ class MyCompletionItem extends vscode.CompletionItem {
        ) {
                super(tsEntry.name, MyCompletionItem.convertKind(tsEntry.kind));

+               // TODO: Remove
+               this.deprecated = true;
+
                if (tsEntry.source) {
                        // De-prioritze auto-imports
                        // https://github.com/Microsoft/vscode/issues/40311
diff --git a/extensions/typescript-language-features/src/typeScriptServiceClientHost.ts b/extensions/typescript-language-features/src/typeScriptServiceClientHost.ts
index 2571d395a5..7a88de73fe 100644
--- a/extensions/typescript-language-features/src/typeScriptServiceClientHost.ts
+++ b/extensions/typescript-language-features/src/typeScriptServiceClientHost.ts
@@ -261,6 +261,7 @@ export default class TypeScriptServiceClientHost extends Disposable {
                if (diagnostic.reportsUnnecessary) {
                        converted.tags = [vscode.DiagnosticTag.Unnecessary];
                }
+               converted.tags = [vscode.DiagnosticTag.Deprecated];
                (converted as vscode.Diagnostic & { reportUnnecessary: any }).reportUnnecessary = diagnostic.reportsUnnecessary;
                return converted as vscode.Diagnostic & { reportUnnecessary: any };
        }
@jrieken

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Please do not mix both features into one PR, please extract the changes for the completion item into a separate PR. Thanks

@jrieken
Copy link
Member

left a comment

one PR, one feature request

@mjbvz

mjbvz approved these changes Jul 25, 2019

Copy link
Contributor

left a comment

The diagnostics tag changes look good to me

@jrieken

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

@kamranayub fyi - I have moved the completion related commits into https://github.com/microsoft/vscode/tree/joh/completionsDeprecated. There is some little work left there and I would appreciate another PR off that

@jrieken jrieken added this to the July 2019 milestone Jul 26, 2019

@jrieken

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

This works like a charm. Thanks @kamranayub

@jrieken jrieken merged commit ff04171 into microsoft:master Jul 26, 2019

5 checks passed

VS Code Build #20190726.22 succeeded
Details
VS Code (Linux) Linux succeeded
Details
VS Code (Windows) Windows succeeded
Details
VS Code (macOS) macOS succeeded
Details
license/cla All CLA requirements met.
Details
@jrieken

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

Merged despite an open question regarding the use of both tags, today deprecated will overrule unnecessary. Let's wait for the bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.