- Notifications
You must be signed in to change notification settings - Fork4.2k
Update F1 keywords to disambiguateclass#48506
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Update F1 keywords to disambiguateclass#48506
Uh oh!
There was an error while loading.Please reload this page.
Conversation
class
davidwengierOct 12, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if(token.IsKind(SyntaxKind.ClassKeyword)){ | |
| if(token.ParentisClassOrStructConstraintSyntax){ | |
| text=Keyword("classconstraint"); | |
| returntrue; | |
| }else{ | |
| text=Keyword("class"); | |
| returntrue; | |
| } | |
| } | |
| if(token.IsKind(SyntaxKind.ClassKeyword)&&token.ParentisClassOrStructConstraintSyntax) | |
| { | |
| text=Keyword("classconstraint"); | |
| returntrue; | |
| } | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can we do the same for struct/new as those are also constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@TheSench feel free to include them in this PR, or not. They are called out indotnet/docs#20799 so we're not going to forget.
They would go to the same file as your current docs PR is changing, so there might be some sense in at least doing that side of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seeing as the build failed due to what appears to be a transient issue, I'll add this and kick off another one. Is there a benefit to creating a new f1_keyword versus just using the existingwhereconstraint_CSharpKeyword one for the constraint form of both of these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm wondering if we want to introduce a switch ontoken.RawKind here now that the number of special cases has grown a bunch. I'll probably be tackling more of these cases in the next week, so that would be something I consider in the next one, but I'd be interested in direction before I get going on it, if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this code is C# specific. So you don't need RawKind (you can use Kind()). I wouldn't really do a switch here. I think the code is clear when it handles each case specifically. There's no need for brevity/perf as this will execute once only on hte rare case of someone hitting f1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the extra context.
TheSench commentedOct 12, 2020
It looks like I missed some styling issues that got caught in the build. I'll look at those later tonight and get them cleaned up. |
davidwengier commentedOct 13, 2020
Thanks for the contribution. I'll wait fordotnet/docs#21037 to be merged before merging this, feel free to ping me here if it happens and I miss the notification. |
6beaafd tobecfcf8CompareTheSench commentedOct 13, 2020
I'm going to need some help figuring out these build failures. I don't see anything helpful in the logs, aside from the possibility that the tests are timing out: |
davidwengier commentedOct 13, 2020
I'm sure its unrelated. I've kicked the integration tests again and will monitor. Thanks for adding in the extra |
TheSench commentedOct 13, 2020
Build passed this time! |
CyrusNajmabadi commentedOct 13, 2020
Thanks! |
Uh oh!
There was an error while loading.Please reload this page.
This PR makes the f1 keywords for the
classkeyword be context-specific. If it appears in a generic constraint clause, it will route to thewhere-generic-type-constraintpage, otherwise it will continue to go to the class declaration page.Fixes part ofdotnet/docs#20799
Related docs PR:
dotnet/docs#21037