Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add AwaitCompletionProvider that makes container Async in VB #54908

Merged
merged 7 commits into from
Jul 29, 2021
Merged

Add AwaitCompletionProvider that makes container Async in VB #54908

merged 7 commits into from
Jul 29, 2021

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jul 18, 2021 •
edited
Loading

Followup to #48352

Unblocks #52289

Youssef1313
Youssef1313 commented Jul 19, 2021
View reviewed changes
Copy link
Member Author

@Youssef1313 Youssef1313 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CyrusNajmabadi This is ready for initial review (modulo integration tests)

@Youssef1313 Youssef1313 marked this pull request as ready for review July 19, 2021 12:46
@Youssef1313 Youssef1313 requested a review from a team as a code owner July 19, 2021 12:46
Youssef1313
Youssef1313 commented Jul 19, 2021
View reviewed changes
src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs Outdated
displayTextSuffix: "",
rules: CompletionItemRules.Default,
Glyph.Keyword,
description: RecommendedKeyword.CreateDisplayParts(text, string.Empty),
Copy link
Member Author

@Youssef1313 Youssef1313 Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description is one existing difference. I kept it the same as before (ie, VB has a tooltip and C# not):

C#:

image

VB:

image

Let me know if you want me to move VBFeaturesResources.Asynchronously_waits_for_the_task_to_finish to the Core layer and have it in both languages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, let's move to core and have same description.

@jinujoseph jinujoseph added Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature Request labels Jul 19, 2021
CyrusNajmabadi
CyrusNajmabadi reviewed Jul 28, 2021
View reviewed changes
src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs Outdated
private protected override bool ShouldMakeContainerAsync(SyntaxToken token)
{
var declaration = GetAsyncSupportingDeclaration(token);
return declaration is not null && !declaration.GetModifiers().Any(SyntaxKind.AsyncKeyword);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if this line coudl be pulled up to the base, using SYntaxGenerator.GetModifiers to unify the code.

CyrusNajmabadi
CyrusNajmabadi reviewed Jul 28, 2021
View reviewed changes
src/Features/Core/Portable/Completion/Providers/AbstractAwaitCompletionProvider.cs

var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
var newText = text.WithChanges(builder);
return CompletionChange.Create(Utilities.Collapse(newText, builder.ToImmutableArray()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i presume this logic moved up effectively unchanged?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

CyrusNajmabadi
CyrusNajmabadi reviewed Jul 28, 2021
View reviewed changes
src/Features/VisualBasic/Portable/Completion/CompletionProviders/AwaitCompletionProvider.vb Outdated

Public Overrides ReadOnly Property TriggerCharacters As ImmutableHashSet(Of Char) = CommonTriggerChars

Private Protected Overrides ReadOnly Property AsyncKeywordTextWithSpace As String = "Async "
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know why, but this bugs me. can jyst you get hte asynckeyword, and hten add the space in the base class? :)

CyrusNajmabadi
CyrusNajmabadi reviewed Jul 28, 2021
View reviewed changes
src/Features/VisualBasic/Portable/Completion/CompletionProviders/AwaitCompletionProvider.vb
SyntaxKind.MultiLineSubLambdaExpression,
SyntaxKind.SingleLineFunctionLambdaExpression,
SyntaxKind.SingleLineSubLambdaExpression
Return DirectCast(declaration, LambdaExpressionSyntax).SubOrFunctionHeader.SubOrFunctionKeyword.SpanStart
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are tehre teests for all these cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CyrusNajmabadi Yes. See VisualBasicCompletionCommandHandlerTests_AwaitCompletion:

  • AwaitCompletionAddsAsync_FunctionDeclaration
  • AwaitCompletionAddsAsync_SubDeclaration
  • AwaitCompletionAddsAsync_MultiLineFunctionLambdaExpression
  • AwaitCompletionAddsAsync_MultiLineSubLambdaExpression
  • AwaitCompletionAddsAsync_SingleLineFunctionLambdaExpression
  • AwaitCompletionAddsAsync_SingleLineSubLambdaExpression

CyrusNajmabadi
CyrusNajmabadi reviewed Jul 28, 2021
View reviewed changes
src/Features/VisualBasic/Portable/Completion/CompletionProviders/AwaitCompletionProvider.vb
End Function

Private Protected Overrides Function GetAsyncSupportingDeclaration(token As SyntaxToken) As SyntaxNode
Return token.GetAncestor(Function(node) node.IsAsyncSupportedFunctionSyntax())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coudl ahve the abstract base calss expose IsAsyncSupportedFunctionSyntax. then more logic can be pushed up.

CyrusNajmabadi
CyrusNajmabadi reviewed Jul 28, 2021
View reviewed changes
...itiesAndExtensions/Workspace/VisualBasic/Extensions/ContextQuery/VisualBasicSyntaxContext.vb Outdated
For Each node In TargetToken.GetAncestors(Of SyntaxNode)()
If node.IsKind(SyntaxKind.SingleLineSubLambdaExpression, SyntaxKind.SingleLineFunctionLambdaExpression,
SyntaxKind.MultiLineSubLambdaExpression, SyntaxKind.MultiLineFunctionLambdaExpression) Then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

CyrusNajmabadi
CyrusNajmabadi approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. some small requests for more unification.

Youssef1313
Youssef1313 commented Jul 29, 2021
View reviewed changes
src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests_AwaitCompletion.vb
@@ -24,7 +24,7 @@ public class C
]]>
</Document>)
state.SendTypeChars("aw")
Await state.AssertSelectedCompletionItem(displayText:="await", isHardSelected:=True, inlineDescription:=CSharpFeaturesResources.Make_container_async)
Await state.AssertSelectedCompletionItem(displayText:="await", isHardSelected:=True, inlineDescription:=FeaturesResources.Make_containing_scope_async)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this existing resource string and went for it (it doesn't seem to be used anywhere else)

Youssef1313
Youssef1313 commented Jul 29, 2021
View reviewed changes
src/Features/Core/Portable/Microsoft.CodeAnalysis.Features.csproj
@@ -54,6 +54,7 @@
<InternalsVisibleTo Include="Roslyn.VisualStudio.Setup" />
<InternalsVisibleTo Include="VBCSCompiler" />
<InternalsVisibleTo Include="AnalyzerRunner" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.EditorFeatures2.UnitTests" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, tests don't have access to FeaturesResources.

@CyrusNajmabadi
Copy link
Member

@Youssef1313 test failures seem real.

@Youssef1313 Youssef1313 changed the title Initial work to have AwaitCompletionProvider in VB Add AwaitCompletionProvider that makes container Async in VB Jul 29, 2021
@CyrusNajmabadi CyrusNajmabadi merged commit 98ea5d0 into dotnet:main Jul 29, 2021
@ghost ghost added this to the Next milestone Jul 29, 2021
@Youssef1313 Youssef1313 deleted the await-completion-vb branch July 30, 2021 01:59
@dibarbet dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Assignees

@CyrusNajmabadi CyrusNajmabadi

Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature Request
Projects
None yet
Milestone
17.0.P4
Development

Successfully merging this pull request may close these issues.
4 participants
@Youssef1313 @CyrusNajmabadi @dibarbet @jinujoseph