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

Re-Analyze project or all projects #3089

Merged
merged 75 commits into from Dec 5, 2019

Conversation

@savpek
Copy link
Contributor

savpek commented May 31, 2019

VScode implementation for OmniSharp/omnisharp-roslyn#1507

Detailed information in OmniSharp/omnisharp-roslyn#1507

image

loading-icon-analyzers

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 7, 2019

Codecov Report

Merging #3089 into master will decrease coverage by 0.07%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3089      +/-   ##
==========================================
- Coverage   89.81%   89.74%   -0.08%     
==========================================
  Files          59       59              
  Lines        1591     1599       +8     
  Branches       89       89              
==========================================
+ Hits         1429     1435       +6     
- Misses        151      153       +2     
  Partials       11       11
Flag Coverage Δ
#integration 100% <ø> (ø) ⬆️
#unit 89.74% <75%> (-0.08%) ⬇️
Impacted Files Coverage Δ
src/omnisharp/EventType.ts 100% <100%> (ø) ⬆️
src/omnisharp/protocol.ts 83.62% <100%> (+0.58%) ⬆️
src/omnisharp/loggingEvents.ts 96.89% <33.33%> (-1.01%) ⬇️

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 0855a2d...640b4cc. Read the comment docs.

1 similar comment
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 7, 2019

Codecov Report

Merging #3089 into master will decrease coverage by 0.07%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3089      +/-   ##
==========================================
- Coverage   89.81%   89.74%   -0.08%     
==========================================
  Files          59       59              
  Lines        1591     1599       +8     
  Branches       89       89              
==========================================
+ Hits         1429     1435       +6     
- Misses        151      153       +2     
  Partials       11       11
Flag Coverage Δ
#integration 100% <ø> (ø) ⬆️
#unit 89.74% <75%> (-0.08%) ⬇️
Impacted Files Coverage Δ
src/omnisharp/EventType.ts 100% <100%> (ø) ⬆️
src/omnisharp/protocol.ts 83.62% <100%> (+0.58%) ⬆️
src/omnisharp/loggingEvents.ts 96.89% <33.33%> (-1.01%) ⬇️

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 0855a2d...640b4cc. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 7, 2019

Codecov Report

Merging #3089 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3089      +/-   ##
=========================================
+ Coverage   89.69%   89.8%   +0.11%     
=========================================
  Files          58      59       +1     
  Lines        1572    1589      +17     
  Branches       89      89              
=========================================
+ Hits         1410    1427      +17     
  Misses        151     151              
  Partials       11      11
Flag Coverage Δ
#integration ?
#unit 89.8% <100%> (+0.11%) ⬆️
Impacted Files Coverage Δ
src/omnisharp/loggingEvents.ts 97.92% <100%> (+0.03%) ⬆️
src/omnisharp/protocol.ts 83.62% <100%> (+0.58%) ⬆️
src/omnisharp/EventType.ts 100% <100%> (ø) ⬆️
src/observers/BackgroundWorkStatusBarObserver.ts 100% <100%> (ø)

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 9f92a69...431326b. Read the comment docs.

savpek added 4 commits Jul 7, 2019
savpek added 2 commits Aug 30, 2019
@savpek savpek requested review from colombod and rchande Sep 10, 2019
@kurobon-jp

This comment has been minimized.

Copy link

kurobon-jp commented Sep 12, 2019

Why isn't a diagnostic event listener implemented?

@savpek

This comment has been minimized.

Copy link
Contributor Author

savpek commented Sep 22, 2019

Why isn't a diagnostic event listener implemented?

It's actually part of LSP implementation i think. It's definetly possible to use those events to sync state of diagnostics between client and server, however i don't think it's in scope of this PR.

CC @david-driscoll @mholo65 can you give better insights of that part @kurobon-jp mentioned? Could it be good idea to consume those events in vscode or should it move LSP directly instead when that is fully ready?

@kurobon-jp

This comment has been minimized.

Copy link

kurobon-jp commented Sep 27, 2019

I think it makes sense apart from including implementation in this PR.

Analyzing thousands of lines of document in Omnisharp can take several seconds.
So I'm thinking of an implementation like this.
名称未設定

This allows users to quickly get important diagnostic results.

savpek added 4 commits Sep 29, 2019
Copy link
Contributor

JoeRobich left a comment

@savpek These changes look good to me. I just had a couple readability related nits. Thanks for implementing this!


// On large workspaces (if maxProjectFileCountForDiagnosticAnalysis) is less than workspace size,
// diagnostic fallback to mode where only open documents are analyzed.
private _validateLargeWorkspace(): NodeJS.Timeout {

This comment has been minimized.

Copy link
@JoeRobich

JoeRobich Nov 22, 2019

Contributor

I feel like the name _validateOpenDocuments is more meaningful here.

if (this._projectValidation) {
this._projectValidation.cancel();
}
private _validateSmallWorkspace(): NodeJS.Timeout {

This comment has been minimized.

Copy link
@JoeRobich

JoeRobich Nov 22, 2019

Contributor

I feel like the name _validateEntireWorkspace is more meaningful here.

await activateCSharpExtension();
await testAssetWorkspace.restore();

This comment has been minimized.

Copy link
@JoeRobich

JoeRobich Nov 22, 2019

Contributor

I noticed this reordering of activation and workspace restore in the tests and was curious. What problem is this solving?

This comment has been minimized.

Copy link
@savpek

savpek Dec 1, 2019

Author Contributor

Don't remember for sure. Its possible that this change is unnecessary since i think latter one where i changed tests to use real restore eventing instead of dummy one was root cause for issues with workspace restore instability during tests.

@JoeRobich

This comment has been minimized.

Copy link
Contributor

JoeRobich commented Nov 27, 2019

@savpek I extracted the test improvements into #3432.

@savpek

This comment has been minimized.

Copy link
Contributor Author

savpek commented Nov 28, 2019

@savpek I extracted the test improvements into #3432.

Thanks, sorry for delay i think i will have time to check these out tomorrow finally 😄

savpek added 3 commits Dec 1, 2019
@savpek

This comment has been minimized.

Copy link
Contributor Author

savpek commented Dec 1, 2019

Broken atm, probably i missed something at merge. I will fix this during upcoming week.

savpek added 3 commits Dec 5, 2019
@savpek

This comment has been minimized.

Copy link
Contributor Author

savpek commented Dec 5, 2019

@JoeRobich i think all concerns fixed now and merge done.

JoeRobich added 2 commits Dec 5, 2019
@JoeRobich

This comment has been minimized.

Copy link
Contributor

JoeRobich commented Dec 5, 2019

@savpek Thanks!

@JoeRobich JoeRobich merged commit 70b37a6 into OmniSharp:master Dec 5, 2019
5 checks passed
5 checks passed
codecov/patch 100% of diff hit (target 89.69%)
Details
codecov/project 89.8% (+0.11%) compared to 9f92a69
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
security/snyk - package.json (david-driscoll) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.