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.

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

Handle more cases in F1 help service #60156

Merged
merged 11 commits into from
Mar 16, 2022

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Mar 14, 2022

@Youssef1313 Youssef1313 requested a review from a team as a code owner March 14, 2022 06:53
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Mar 14, 2022
@@ -239,6 +232,12 @@ private static bool TryGetTextForOperator(SyntaxToken token, Document document,
return true;
}

if (token.IsKind(SyntaxKind.LessThanToken, SyntaxKind.GreaterThanToken) && token.Parent.IsKind(SyntaxKind.TypeParameterList, SyntaxKind.TypeArgumentList))
{
text = Keyword("generics");
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'm using the Keyword call for consistency, but let me know if I shouldn't use it in this case.

@Youssef1313 Youssef1313 changed the title Handle '<' and '>' tokens in generic context for F1 service Handle more cases in F1 keyword service Mar 14, 2022
@Youssef1313 Youssef1313 changed the title Handle more cases in F1 keyword service Handle more cases in F1 help service Mar 14, 2022
Copy link
Member Author

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

@RikkiGibson FYI we will need to handle !!. But it's waiting on dotnet/docs#28443. Do you want a tracking issue for this?

@@ -239,6 +287,12 @@ private static bool TryGetTextForOperator(SyntaxToken token, Document document,
return true;
}

if (token.IsKind(SyntaxKind.LessThanToken, SyntaxKind.GreaterThanToken) && token.Parent.IsKind(SyntaxKind.TypeParameterList, SyntaxKind.TypeArgumentList))
Copy link
Member Author

@Youssef1313 Youssef1313 Mar 14, 2022

Choose a reason for hiding this comment

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

Note to self:

TODO:

< and > in:

  • FunctionPointerParameterListSyntax
  • RelationalPatternSyntax
  • XML doc comments (OperatorMemberCrefSyntax, XmlElementStartTagSyntax, XmlEmptyElementSyntax)

@davidwengier @CyrusNajmabadi Is it worth handling < and > in XML doc comments?

@@ -50,14 +51,15 @@ public override async Task<string> GetHelpTermAsync(Document document, TextSpan
var semanticModel = await document.ReuseExistingSpeculativeModelAsync(span, cancellationToken).ConfigureAwait(false);

var result = TryGetText(token, semanticModel, document, cancellationToken);
if (string.IsNullOrEmpty(result))
if (result is null)
Copy link
Member

Choose a reason for hiding this comment

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

why this cahnge?

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 I changed the path that returns string.Empty to return null instead. Just simplifies the check and allows me to use ?? below. We're also now more consistent. Previously we were returning null in some cases, and string.Empty in other cases. Both had the same meaning of "we don't have an F1 keyword".

{
var previousToken = token.GetPreviousToken();
if (previousToken.Span.IntersectsWith(span))
result = TryGetText(previousToken, semanticModel, document, cancellationToken);
}

return result;
// This redirects to https://docs.microsoft.com/visualstudio/ide/not-in-toc/default, indicating nothing is found.
return result ?? "vs.texteditor";
Copy link
Member

Choose a reason for hiding this comment

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

can you make a constant for this at the top of the file and doc that?

Comment on lines 295 to 296
// PROTOTYPE: Figure out where to redirect.

Copy link
Member Author

Choose a reason for hiding this comment

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

@BillWagner @333fred Is there a documentation page we can redirect to here?

Copy link
Member

Choose a reason for hiding this comment

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

Not yet. I'll ping you and @RikkiGibson on the docs issue referenced below when I open that PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@BillWagner Sorry for not clarifying. This was about function pointers

if (token.IsKind(SyntaxKind.LessThanToken, SyntaxKind.GreaterThanToken))
{
if (token.Parent.IsKind(SyntaxKind.FunctionPointerParameterList))
{
// PROTOTYPE: Figure out where to redirect.

I found now that https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/unsafe-code is the most suitable page.

Opened dotnet/docs#28669.

@RikkiGibson
Copy link
Contributor

@RikkiGibson FYI we will need to handle !!. But it's waiting on dotnet/docs#28443. Do you want a tracking issue for this?

No thanks, I'll just subscribe to the docs issue.

@davidwengier davidwengier enabled auto-merge (squash) March 16, 2022 00:13
@davidwengier davidwengier merged commit 8e31456 into dotnet:main Mar 16, 2022
@ghost ghost added this to the Next milestone Mar 16, 2022
@Youssef1313 Youssef1313 deleted the 60154-generics-f1 branch March 16, 2022 02:33
@allisonchou allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
6 participants