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

Fadeout based on roslyn tag 'unnecessary'. #2873

Merged
merged 60 commits into from
Jun 10, 2019

Conversation

savpek
Copy link
Contributor

@savpek savpek commented Feb 24, 2019

Based on original PR #2646 from @akshita31

Requires OmniSharp/omnisharp-roslyn#1410 and OmniSharp/omnisharp-roslyn#1076 to be fully functional.

Without backend support (previous PR:s), this has few hard coding to fade most common situations without full blown analysis: (decided not to hard code anything at this point, requires analyzers.)
image

With backend support (theres Roslynator package that fadeouts some additional items from image):
image

Theres some mapping when fadeout occurs because VSCode doesn't support 'hidden' with fadeout, so those items must be converted to hints or fadeout doesn't work on them.

@savpek savpek changed the title Fadeout based on roslyn tag 'unused'. [WIP] Fadeout based on roslyn tag 'unused'. Feb 24, 2019
@codecov
Copy link

codecov bot commented Feb 24, 2019

Codecov Report

Merging #2873 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2873   +/-   ##
=======================================
  Coverage   89.74%   89.74%           
=======================================
  Files          59       59           
  Lines        1570     1570           
  Branches       89       89           
=======================================
  Hits         1409     1409           
  Misses        149      149           
  Partials       12       12
Flag Coverage Δ
#integration 100% <ø> (ø) ⬆️
#unit 89.74% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/omnisharp/protocol.ts 83.03% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0a0336...5c3b0b2. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 24, 2019

Codecov Report

Merging #2873 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2873   +/-   ##
=======================================
  Coverage   89.79%   89.79%           
=======================================
  Files          59       59           
  Lines        1588     1588           
  Branches       89       89           
=======================================
  Hits         1426     1426           
  Misses        151      151           
  Partials       11       11
Flag Coverage Δ
#integration ?
#unit 89.79% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/omnisharp/protocol.ts 83.03% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dfe3ca...cfad848. Read the comment docs.

@savpek savpek changed the title [WIP] Fadeout based on roslyn tag 'unused'. [WIP] Fadeout based on roslyn tag 'unnecessary'. Feb 25, 2019
@savpek savpek changed the title [WIP] Fadeout based on roslyn tag 'unnecessary'. Fadeout based on roslyn tag 'unnecessary'. Mar 20, 2019
@savpek
Copy link
Contributor Author

savpek commented Mar 20, 2019

This is ready for review @rchande @akshita31

Requires latest version of omnisharp-roslyn master branch and analyzers enabled.

@filipw
Copy link
Contributor

filipw commented Mar 20, 2019

I would recommend to wait with this until there is official omnisharp-roslyn with the analyzers feature + that version is consumed by the C# extension.
Otherwise there is a setting that only works for "omnisharp.path": "latest" which is a bit confusing.

@akshita31 akshita31 self-requested a review April 5, 2019 18:47
@akshita31
Copy link
Contributor

@savpek Is it possible to add a test for this ?

@savpek
Copy link
Contributor Author

savpek commented Jun 1, 2019

Could @rchande @akshita31 check this out? It should be good to go and i use this at daily basis (custom build).

I'd like to use this PR test infra with #3089 since i ended up refactor diagnostic provider there a bit and it's good point to add more tests for diagnostics 🙂

@akshita31
Copy link
Contributor

Looks good to me. Thanks @savpek

@akshita31
Copy link
Contributor

@savpek Would you like to do another PR to add the changelog.

savpek added a commit to savpek/omnisharp-vscode that referenced this pull request Jun 9, 2019
@savpek
Copy link
Contributor Author

savpek commented Jun 9, 2019

@akshita31 changelog #3105

@akshita31 akshita31 requested a review from rchande June 9, 2019 21:55
@akshita31 akshita31 merged commit b1a040a into dotnet:master Jun 10, 2019
@savpek savpek deleted the feature/fadeout-with-roslyn-diag branch June 10, 2019 17:54
rchande pushed a commit that referenced this pull request Jun 11, 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.

None yet

4 participants