Classification API #13741
Classification API #13741
Conversation
| using System.Linq; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.CodeAnalysis.DocumentationComments; |
There was a problem hiding this comment.
You came by my office to tell me I was trolling you, but I wasn't. Note that the usings below this comment aren't sorted. 😛
|
@CyrusNajmabadi just pointing out the minimal extension points I would like to have at this time. ;) |
| _spanSet = spanSet; | ||
| } | ||
|
|
||
| public void AddClassification(ClassifiedSpan classifiedSpan) |
There was a problem hiding this comment.
I think, before we make this public, we shouldl just make this be 'TaggedText'. Or possibly TaggedSpan. I think using 'Tags' here makes sense so we have a single place where we define all the tags. Then, internally, we could copy this over to the editor terminology.
What do you think?
There was a problem hiding this comment.
I like that idea! However, the Classifier API is already public and has been so since 1.0. ClassifiedSpan itself is public right here: http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Classification/ClassifiedSpan.cs,9348503c8c058a4b.
|
Sorry if we conflicted. I would recommend taking what is in Master as it includes a bunch of fixes around Classification Merging that was causing bug sin the new streaming FAR work. |
|
We should create some bugs to track:
Note: because of the repetitive pattern for exception catching, we probably want to just do this with some common code that all our services can call into. |
|
I can understand wanting to avoid the lambda allocation. Since we're using exception filters though, we could just avoid the catch case for 'OperationCanceled' and just have the normal 'Exception' block handle that in the filter it calls. |
|
@CyrusNajmabadi the catch for OperationCanceled was not necessary. It had just come with the cut & paste job. |
| @@ -182,6 +181,8 @@ | |||
| <PublicAPI Include="PublicAPI.Shipped.txt" /> | |||
| <PublicAPI Include="PublicAPI.Unshipped.txt" /> | |||
| </ItemGroup> | |||
| <ItemGroup /> | |||
| <ItemGroup> | |||
| <Folder Include="Classification\" /> | |||
There was a problem hiding this comment.
Should this folder be deleted from the project now?
| @@ -217,6 +216,7 @@ | |||
| </EmbeddedResource> | |||
| </ItemGroup> | |||
| <ItemGroup> | |||
| <Folder Include="Classification\" /> | |||
There was a problem hiding this comment.
Can this folder be deleted now?
Pilchie
added
Area-IDE
Concept-API
|
We apologize, but we are closing this PR due to code drift. We regret letting this PR go so long without attention, but at this point the code base has changed too much for us to revisit older PRs. Going forward, we will manage PRs better under an SLA currently under draft in issue #26266 – we encourage you to add comments to that draft as you see fit. Although that SLA is still in draft form, we nevertheless want to move forward with the identification of older PRs to give contributors as much early notice as possible to review and update them. If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you! |
jaredpar
added
the
Resolution-Expired
@Pilchie @CyrusNajmabadi @DustinCampbell @dpoeschl @jasonmalinowski @rchande please take look and comment on this draft change to the Classification Service API.
The interesting things that happened here is that the editor layer service got removed, and the workspace layer service got refactored. A change to take note of is that the GetClassificationsXXXAsync API's now return an ImmutableArray of ClassifiedSpan's instead of passing a list in. It appears this list argument enabled the API to optimally not cause any allocations, or quite few. With the change, the returned list is allocated each time the API is used. Of course, this needed to change for a variety of reasons, such as potentially using the API cross process boundaries and the fact that async methods with out-like args is an easy pit of failure for users.
Note, the provider/classifier model has not been changed and has been left as a C#/VB only implementation detail, since it depends on syntax trees and symbols. It is not intended to be made public.