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

Proposal to add roslyn 'tags' to diagnostic response #1410

Merged
merged 11 commits into from
Mar 19, 2019

Conversation

savpek
Copy link
Contributor

@savpek savpek commented Feb 24, 2019

Tags contains metadata from compiler like 'unused', 'telemetry', 'compiler' and so on. Interesting bit at this point is unused tag that can be used to implement 'fadeout' feature for unused code.

Feature like:
image

Does this api change looks good @rchande @filipw @david-driscoll @DustinCampbell ?

Theres multiple options to surface this on model, i think another option is to add "isFaded" flag or something. But personally i like this tag system better since it can support other scenarios as well. And it seems to be common way to deal this since both VSCode and roslyn implement same behavior.

To get actual value from this change this needs #1076. I don't know any CS diagnostics that hides anything, they seems to be IDE analyzers.

@filipw
Copy link
Member

filipw commented Feb 25, 2019

I like this idea. Calling it tags also makes sense to me, as it's a known concept and already called like that in both Roslyn in VS Code.
I think, however, that it might add some unnecessary overhead to always report all of those diagnostic tags for everything, especially as most of it doesn't add much value for now.

I'd suggest we could include the tags property into the API already but start by just reporting Unnecessary values for now. We can opt-in more stuff later if needed or there is a use case.

Also, it would be good to add a test, for example here https://github.com/OmniSharp/omnisharp-roslyn/blob/master/tests/OmniSharp.Roslyn.CSharp.Tests/DiagnosticsFacts.cs to make sure we don't break this (since it's not typed and served as a string array it's susceptible to easy breaking).

@savpek
Copy link
Contributor Author

savpek commented Feb 25, 2019

Good points. I added few tests and tag filtering which passes only Unnessery tag for client.

However without analyzers the 'happy path' with unnecessary tag is nonexistent. I added placeholder test for it to be implemented once analyzers are available.

One question: Should we add Unnesessary tag for few selected CS warning like unnesessary import? This isn't implemented by roslyn but it gives bit extra to users without analyzers enabled.

@SirIntruder
Copy link
Contributor

VSCode march update got a cool new feature to mark code fixes as preferred - preferred action is autoselected in the dropdown and can be triggered with a hotkey. Does roslyn provide a "preferred" tag or something that can be used to that effect?

@savpek
Copy link
Contributor Author

savpek commented Mar 13, 2019

I think there isn't such tag in diagnostics results from Roslyn. However if there's fix all provider available for diagnostic, it could be reasonable assumption that it could work as preferred code action. This is something which could be prototyped to see if it works well 🤔

Does visual studio impelement some kind of preferred code action feature? Roslyn has most of it's features implemented in visual studio IDE so if it has feature like VSCode then i think it's definetly doable in VScode too.

@david-driscoll
Copy link
Member

Blocked waiting on #1076

@savpek
Copy link
Contributor Author

savpek commented Mar 19, 2019

This is ready for review now @filipw @david-driscoll @SirIntruder

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

good to go - thanks @savpek!

@savpek
Copy link
Contributor Author

savpek commented Mar 19, 2019

@filipw build ready (appvoyer was very slow for some reason) and passing. Could you merge this? I'd like to continue with dotnet/vscode-csharp#2873 🙂

@filipw filipw removed the blocked label Mar 19, 2019
@filipw filipw merged commit ae11b6e into OmniSharp:master Mar 19, 2019
@filipw
Copy link
Member

filipw commented Mar 19, 2019

🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants