Add code fix for VSSDK001 #15
Conversation
| .WithTrailingTrivia(baseTypeSyntax.GetTrailingTrivia()); | ||
| var updatedRoot = root.ReplaceNode(baseTypeSyntax, asyncPackageBaseTypeSyntax); | ||
|
|
||
| return document.WithSyntaxRoot(updatedRoot); |
There was a problem hiding this comment.
How flexible are we here? Can we also add the InitializeAsync method with it throwing NotImplementedException?
There was a problem hiding this comment.
I'm working on that now, actually.
|
|
||
| if (qualifiers.Count == 0) | ||
| { | ||
| throw new ArgumentException("At least one qualifier required."); |
There was a problem hiding this comment.
Should pass in nameof(qualifiers) as the second arg to specify the argument causing the issue
|
|
||
| private async Task<Document> ChangeBaseTypeAsync(Document document, Diagnostic diagnostic, CancellationToken cancellationToken) | ||
| { | ||
| var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Can extension developers enable these analyzers on VS 2013 projects without referecing Shell.14.0 or Shell.15.0 in which case this fix or the original warning wouldn't apply to them?
There was a problem hiding this comment.
Yes they can, and I haven't accommodated for that yet.
There was a problem hiding this comment.
Yeah, this one is interesting to me too. If a vsix project targets VS 2013 or below, this diagnostic shouldn't apply. They can choose to simply ignore the diagnostic but I wonder if it's possible to be smarter. Maybe looking at the references of the current project and determining if the version of Shell they're referencing is Shell.14+.
There was a problem hiding this comment.
There is a solution, yes. We could look at references but that would get tedious to maintain long-term and isn't a great pattern IMO. Instead, I plan to support this by testing for the existence of the type within that compilation unit. If it exists, the analyzer will run -- otherwise it will turn itself off. This should scale and be a pattern that other analyzers can use too.
|
There are conflicts. Perhaps due to the merger of the previous 3 PRs |
| SyntaxFactory.MemberAccessExpression( | ||
| SyntaxKind.SimpleMemberAccessExpression, | ||
| Types.ThreadHelper.TypeSyntax, | ||
| SyntaxFactory.IdentifierName(Types.ThreadHelper.JoinableTaskFactory)), |
There was a problem hiding this comment.
Why not use this.JoinableTaskFactory here instead? That's what we're planning on doing in the AsyncPackage template when we get to it.
There was a problem hiding this comment.
Given Mads already completed the PR, I have a commit that addresses this that I will include in a future PR.
Codecov Report
@@ Coverage Diff @@
## master #15 +/- ##
===========================================
+ Coverage 65.45% 87.43% +21.98%
===========================================
Files 2 5 +3
Lines 55 191 +136
===========================================
+ Hits 36 167 +131
- Misses 19 24 +5
Continue to review full report at Codecov.
|
|
Awesome. Thanks! |
|
Sorry, it seems that I got too excited and merged the PR prematurely :) |
This automatically changes the base type from
PackagetoAsyncPackageIn the future, it can apply more changes like update the Initialize override, etc.
This is a rather large change because with it comes the testing infrastructure to support testing code fix providers.