-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Public APIs for opened and closed event handling for non-source documents #61523
Comments
So as far as which option seems best, nothing really jumps out other than thinking about what use cases we see. Chatting with @shyamnamboodiripad he was imagining that they only needed to track additional files for Razor scenarios and .editorconfig files were unimportant since they're not running anlayzers. However, technically .editorconfig files can be read by a generator which could change the generated output too, so I think any handling they do for additional files may also apply to .editorconfig files too, although I won't know the fuller details. @shyamnamboodiripad Does that sound right or did I mangle something here? |
Yes @jasonmalinowski, that is correct. I don't have a strong preference between the options. However, I like Proposal 3. It seems like a good longer term direction to have a single API that can handle all types of documents especially if we deprecate the current DocumentOpened / DocumentClosed APIs. (Even otherwise, the duplicate notifications should be easy to explain with good documentation comments which explain that DocumentOpened and DocumentClosed are preserved for backwards compatibility and that all new usages should prefer the new TextDocumentOpened and TextDocumentClosed.) If we can't choose Proposal 3, I would prefer Proposal 1 over Proposal 2. Proposal 2 treats non-source documents specially and may be problematic if we end up introducing a new document type in the future that is not a Document but could still somehow be considered a 'source' document. It would also be preferrable to avoid the need for a new categorization of documents (source v/s non-source) that is only relevant in this API, but is not relevant in other parts of the Roslyn API surface. Proposal 1 would force us to introduce new APIs for every future document type (which we hope will never happen :)). However, the resulting APIs would always be intuitive to consumers. This is probably also the easiest to explain alternative amongst the three options above. |
…er until we are ready to make the APIs public (tracked with dotnet#61523)
@mavasani @jasonmalinowski I just discovered a couple of things as I was trying out the internal API and thought I would mention them here for consideration.
Nice to haves: The following may be out-of-scope for current proposal but would be nice to support so that it is possible to do a little more with
|
API Review Notes
|
@shyamnamboodiripad To your concerns there we should get follow-ups on those; we didn't think it in any way impacted this API design.
What sort of information did you want to imagine and where would it come from? Note that since we do serialization/cross process sync we'd have to think carefully about the types involved here.
I would expect they do, were you able to confirm that? |
…ocuments Public APIs were approved in dotnet#61523 (comment). NOTE: Once the Unit testing team moves to the new public APIs, we can delete the corresponding ExternalAccess layer APIs which were added as a temporary workaround.
@jasonmalinowski At the very least it would be great to expose the
The main thing I was interested in knowing for my feature is whether the document in question is a razor file (i.e.,
Yes, as I hinted in an "Update" to my comment above, turns out most of the APIs I listed above do support |
Background and Motivation
We currently have a few workspace level public APIs and events for source documents being opened/closed in the workspace. This issue proposes adding similar APIs and events for non-source documents (additional documents and analyzer config documents).
Current API
Current public APIs for source documents opened/closed events:
Proposed API
There are 3 possible API shapes that we can use to expose these events for non-source documents.
Proposal 1
Replicate each of the above APIs for every non-source document type, i.e. have distinct and duplicated APIs for
AdditionalDocument
andAnalyzerConfigDocument
. This is the API shape chosen in the current PR: https://github.com/dotnet/roslyn/pull/61377/filesProposal 2
Introduce a single set of APIs for non-source text documents, i.e. have shared APIs for
AdditionalDocument
andAnalyzerConfigDocument
, and any other future non-source document type that we may add.Proposal 3
Introduce a single set of APIs for all (source and non-source) text documents, i.e. have shared APIs for
Document
,AdditionalDocument
andAnalyzerConfigDocument
, and any other future document type that we may add.This last proposal will lead to us generating duplicate events for source documents, existing
DocumentOpened
andDocumentClosed
events, plus the newTextDocumentOpened
andTextDocumentClosed
events. We can consider deprecating the existing APIs/events specific to source documents, if we feel that the duplication would be confusing OR we can retain the existing APIs for clients interested in only source document opening/closing.The text was updated successfully, but these errors were encountered: