Simplify our use of an extension.#26670
Conversation
|
Tagging @dotnet/roslyn-ide @jcouv |
| } | ||
|
|
||
| public TService GetLanguageService<TService>() where TService : class, ILanguageService | ||
| => this.Document.GetLanguageService<TService>(); |
There was a problem hiding this comment.
Is this really needed? This only saves about 9 characters.
There was a problem hiding this comment.
The benefit here is mostly around the confusion that i run into a lot when trying to use this 'extension' on a 'document' that turns out to actually be a SemanticDocument. i.e. i do:
document.GetLanguageService<ISyntaxFactsService>();it doesn't work.
i try to add using. it doesn't work. i think something is broken. I then have to poke aaround and realize "oh right, it's this strange type called 'XXXDocument' which isn't actually a Document :)
I personally think it's worth it just for the annoyance factor.
There was a problem hiding this comment.
I personally think every time you have a SemanticDocument you should name it semanticDocument rather than document to avoid the confusion. It's still a different thing a you need to be aware of that.
There was a problem hiding this comment.
I prefer this solution.
There was a problem hiding this comment.
I guess I just don't see this as a sustainable approach. Are we going to copy all methods and extensions that Document has and forward them?
There was a problem hiding this comment.
Are we going to copy all methods and extensions that Document has and forward them?
If there are other extensions that we commonly run into problems with? Sure.
It's less an issue with non-extensions because, of course, intellisense just works.
I guess I just don't see this as a sustainable approach.
We can address things on a case by case basis. This is an internal API and we can define it to be as pleasant as necessary for our own use cases.
There was a problem hiding this comment.
I'm with @Neme12: if your semanticDocuments are called document, fix that. 😄
| <PackageReference Include="System.Threading.Tasks.Extensions" Version="$(SystemThreadingTasksExtensionsVersion)" /> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <Folder Include="RemoveUnusedMembers\" /> |
There was a problem hiding this comment.
Hrmm.. strange. That was work in another branch. i wonder how that happened. Will address.
jasonmalinowski
left a comment
There was a problem hiding this comment.
The code looks OK, but I despise these sorts of forwarders as they create a maintenance burden. Invoking find references on the "real" method or the extension now will find probably half of all of the hits instead of all of them. I've lost more time on these than they've ever saved me.
Ok. will remove. |
|
@jasonmalinowski Have removed forwarding methods. Renamed vars to "semanticDocument" as appropriate. |
| { | ||
| return s => CodeGenerationSymbolFactory.CreateParameterSymbol( | ||
| default(ImmutableArray<AttributeData>), RefKind.None, isParams, GetTypeSymbol(s.Compilation, typeFullName, typeArrayRank), parameterName, | ||
| default, RefKind.None, isParams, GetTypeSymbol(s.Compilation, typeFullName, typeArrayRank), parameterName, |
There was a problem hiding this comment.
Not against this, but I do have to admit the old form actually was clearer: it was clear we were creating a parameter with no attributes, as opposed to, well, now it's a default...something?
There was a problem hiding this comment.
will fix up!
There was a problem hiding this comment.
Have tweaked things.
|
test windows_release_vs-integration_prtest |
|
@jinujoseph @jasonmalinowski this can be merged in. |
|
@jinujoseph: ask mode approval for this? |
|
Approved to merge for 15.8.Preview2 |
|
Thanks! |
No description provided.