refactor diagnostic analyzer service v1 state management code - #930
Conversation
and added place holder for state manager
all tests passing. now StateManager owns all state related operation.
|
@srivatsn @mavasani @shyamnamboodiripad can you take a look? |
There was a problem hiding this comment.
Why? I'd thought we explicitly want to de-duplicate. Is the suggestion here that situation is too contrived to be assumed realistic?
I could accidentally come up in a situation with duplicate analyzer references. A project nuget reference might be pulling in analyzer and it might also be installed in the box on some machine which wants specific analyzer enabled for all development. I don't want to remove project nuget reference if not all of machines on which project is developed on has VSIX or host analyzer.
Is it non-trivial to de-duplicate here? Else, we should just do it.
There was a problem hiding this comment.
@mavasani that de-duplication will work. so, having same analyzer reference (basically same file) in multiple places regardless of layer (host, project) works.
the one I removed is same instance of diagnostic analyzer in two different analyzer reference. for this to happen, people has to do what the test did. create custom analyzer references and put same instance of diagnostic analyzer in multiple references.
I don't think that is something we need to care.
There was a problem hiding this comment.
Agreed, thanks for the explanation.
some code clean up - rename and such
There was a problem hiding this comment.
Your point 2. is not possible in VS scenario with just analyzer file references, but possible through our Workspace APIs creating AnalyzerImageReferences. Probably can ignore it, but I presume it broke some tests?
There was a problem hiding this comment.
ya, the test you have put some comment above about de-duplication. I think if a user deliberately did that, they will get duplicated diagnostic.
There was a problem hiding this comment.
Why was this requireUpToDateDocumentDiagnostic check removed? did it move somewhere else? diff view is not enabling me to figure out what happened here.
There was a problem hiding this comment.
since state can't never be null now.
more clean up after feedback
|
Looks fine to me. |
|
@mavasani manish? |
|
Looks good to me too. Do you want to go first or let me go first? :) |
|
Hmm. I thought you already did. How I got merge conflict…
From: Manish Vasani [mailto:notifications@github.com] Looks good to me too. Do you want to go first or let me go first? :) — |
|
Nope, probably from previous checkin. I am just running final test.cmd after addressing review feedback. |
refactor diagnostic analyzer service v1 state management code
5 participants
refactored diagnostic analyzer service v1 state management code.
removed all AnalyzerAndState types and split its responsibilities to HostAnalyzerManager and StateManager.
now StateManager owns everything related to state and it now has 4 APIs that will either move state forward, remove state, get cached data or get latest data.