Remove IDocumentSnapshot.Project
#9854
Conversation
The eco-system specific property is okay to have, and is used internally in the document snapshots, but exposing it externally makes it hard to legacy the important bits of IProjectSnapshot out
There was a problem hiding this comment.
I'll be honest, I don't love this change. I understand what you're trying to do, but it doesn't feel right to me. It's a significant change in semantics to say that a document snapshot, which is supposed to be an immutable object that is part of the data that makes up a project snapshot, another immutable object, no longer knows what project snapshot that it came from. Something about this really doesn't sit right with me and the number of changes required to support it really smells fishy to me. Could we discuss this approach further before pushing it in?
| { | ||
| ClearClosedDocumentsPublishedDiagnostics(PublishedRazorDiagnostics); | ||
| ClearClosedDocumentsPublishedDiagnostics(PublishedCSharpDiagnostics); | ||
| lock (PublishedCSharpDiagnostics) |
There was a problem hiding this comment.
Having nested locks at the same indent level with just one set of braces is pretty standard. However, if you'd rather indent the nested lock statement, please add braces for readability.
There was a problem hiding this comment.
This is 100% not intentional, I just have a habit of pressing Ctrl+E, D a fair amount. Will fix up, thanks for catching it.
of course! |
Part of #9519
In trying to reduce the suface area of
IProjectSnapshotfor cohosting I ran into a problem of the somewhat circular dependency between document and project snapshots. eg, I want to splitIProjectSnapshotinto two types:IProjectSnapshotwhich has the minimum needed to reason about a project and its documents, andIProjectSnapshotThatCanCompileRazorFilesfor the existing Razor project system, where the project snapshot also has to be able to produce generated code. Obviously that name is not final :)The problem comes when someone asks a
IProjectSnapshotThatCanCompileRazorFilesfor any documents, they get back anIDocumentSnapshot, and if they pass that around and something wants to know about the project it came from, they just get anIProjectSnapshotand have potentially lost the extra information that it original came from. This could be solved via runtime validation, and that probably would work perfectly well, but in this case because cohosting is off-by-default and still very early in development, I wanted the reassurance that the compiler gives by making this a compile time check. This could also have been solved with generics viaIDocumentSnapshot<TProject>but I can't imagine that would have ended well.It turns out that there were very few spots that needed the project from the
IDocumentSnapshot, and in every case the source of the document snapshot had the project snapshot right there and available too, so removing the property seemed the most straight forward way of resolving this. Other than some minor Moq annoyance (and who doesn't get annoyed at Moq every now and again) the changes here are pretty much just all plumbing, but looking commit-at-a-time might be a little clearer as to what is going on.