Skip to content

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Nov 4, 2022

Fixes #65210

@CyrusNajmabadi I'm not familiar with F1 help. I just picked a keyword "defaultconstraint" that seems aligned with existing keywords. If I understood correctly, we can ping Bill to add that keyword and point it appropriately?

@jcouv jcouv added the Area-IDE label Nov 4, 2022
@jcouv jcouv self-assigned this Nov 4, 2022
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.

yup yup , that's it.

@Youssef1313
Copy link
Member

I just picked a keyword "defaultconstraint" that seems aligned with existing keywords. If I understood correctly, we can ping Bill to add that keyword and point it appropriately?

I opened dotnet/docs#32184 to do that.

Copy link
Member

@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.

LGTM

Comment on lines 456 to 466
if (token.Parent is DefaultSwitchLabelSyntax)
{
text = Keyword("defaultcase");
return true;
}

if (token.Parent is DefaultConstraintSyntax)
{
text = Keyword("defaultconstraint");
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Consider handling goto default (not sure which doc page is suitable).

Also #line default

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestions. Added

I'll let @BillWagner decide, but for goto default we could always go to the page for switch and for #line default we could go to the section for #line preprocessing directive.

Copy link
Member

@Youssef1313 Youssef1313 Nov 4, 2022

Choose a reason for hiding this comment

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

we could go to the section

We can't go to a specific section, unfortunately.

But I'll update the page on dotnet/docs to have the new F1 keyword. Edit: Opened dotnet/docs#32197

@jcouv jcouv marked this pull request as ready for review November 4, 2022 17:42
@jcouv jcouv requested a review from a team as a code owner November 4, 2022 17:42
@jcouv jcouv requested a review from CyrusNajmabadi November 4, 2022 17:42

if (token.Parent is DefaultSwitchLabelSyntax or GotoStatementSyntax)
{
text = Keyword("defaultcase");
Copy link
Member

Choose a reason for hiding this comment

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

This will redirect to https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/selection-statements

Currently there doesn't seem to be anything about goto default there, but I opened dotnet/docs#32196

Comment on lines 456 to 461
if (token.Parent is DefaultSwitchLabelSyntax)
{
text = Keyword("defaultcase");
return true;
}

Copy link
Member

@Youssef1313 Youssef1313 Nov 4, 2022

Choose a reason for hiding this comment

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

Duplicated below

Suggested change
if (token.Parent is DefaultSwitchLabelSyntax)
{
text = Keyword("defaultcase");
return true;
}

Comment on lines +474 to +478
if (token.Parent is LineDirectiveTriviaSyntax)
{
text = Keyword("defaultline");
return true;
}
Copy link
Member

@Youssef1313 Youssef1313 Nov 4, 2022

Choose a reason for hiding this comment

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

@BillWagner Where should this go? The proposal? Or is there any conceptual doc for it? Just saw #65229 (comment)

@jcouv
Copy link
Member Author

jcouv commented Nov 7, 2022

@CyrusNajmabadi Made a couple additions since your review. Could you take another look?

@CyrusNajmabadi
Copy link
Member

thanks!

@jcouv jcouv merged commit ee81de1 into dotnet:main Nov 7, 2022
@jcouv jcouv deleted the default-f1 branch November 7, 2022 19:24
@ghost ghost added this to the Next milestone Nov 7, 2022
@allisonchou allisonchou modified the milestones: Next, 17.5 P2 Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

F1 on default in generic constraints takes to wrong documentation

4 participants