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

Document SyntaxKinds #47257

Merged
merged 16 commits into from
Sep 8, 2020
Merged

Document SyntaxKinds #47257

merged 16 commits into from
Sep 8, 2020

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Aug 29, 2020

After completing, it should fix dotnet/docs#20344 (this will be in a follow-up PR).

I added XML docs for the first few SyntaxKinds that I know, and kept a TODO for what I don't know.

Steps in my mind to complete this:

  1. Review the currently few added summaries to see if you like the current wording or not.
  2. After that, I'll complete documenting the whole enum, keeping TODOs for things I don't know.
  3. Get feedback about the TODOs.
  4. Addressing feedback.

@Youssef1313 Youssef1313 requested a review from a team as a code owner August 29, 2020 10:14
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.

I don't think this is a good idea. The point is that the tokens don't represent anything. They're just tokens. A * could appear anywhere. The kind just tells you what textual form it has. You can tell what it means (i.e. if it's multiplication, or pointer type, or pointer deref, or funcptr, etc.) From the syntax and semantic operations available.

@CyrusNajmabadi
Copy link
Member

Or, if you are going to doc the token kinds, they should be more like: represents a ! token

@Youssef1313
Copy link
Member Author

@CyrusNajmabadi Thanks for review!
I'll update the tokens doc with your suggestion.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Aug 29, 2020

@CyrusNajmabadi I've added more XML docs. Can you review what I've so far?

I think some keywords are for compiler usage only, but I'm not aware of all of them. Can you point to them to add a note indicating that?

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

The closing </c> tags are currently malformed

src/Compilers/CSharp/Portable/Syntax/SyntaxKind.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Syntax/SyntaxKind.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Syntax/SyntaxKind.cs Outdated Show resolved Hide resolved
Youssef1313 and others added 2 commits August 29, 2020 21:06
@Youssef1313 Youssef1313 marked this pull request as draft August 30, 2020 11:24
@Youssef1313 Youssef1313 marked this pull request as ready for review August 30, 2020 11:24
Merge master into patch-7
@Youssef1313 Youssef1313 marked this pull request as draft August 30, 2020 12:01
Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>
@sharwell
Copy link
Member

@Youssef1313 Looks like we need a using directive for Microsoft.CodeAnalysis.CSharp.Syntax.

@Youssef1313
Copy link
Member Author

@sharwell I'll add it when I continue documenting the rest of the enum. There are still many undocumented members. Or do you want to merge it as is now and open a follow-up a PR?

@sharwell
Copy link
Member

@Youssef1313 I'd be fine with including the part we have here and looking at the next set in a follow-up.

@@ -4,6 +4,8 @@

#nullable enable

using Microsoft.CodeAnalysis.CSharp.Syntax;
Copy link
Member Author

Choose a reason for hiding this comment

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

@sharwell, CI is failing here with error CS0234 The type or namespace name 'Syntax' does not exist in the namespace 'Microsoft.CodeAnalysis.CSharp'

This doesn't happen with me in Visual Studio.

Copy link
Member

@sharwell sharwell Sep 1, 2020

Choose a reason for hiding this comment

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

I believe this is occurring because SyntaxKinds.cs is linked into the code generation projects. You may need to wrap this in #if so it's only used for the Microsoft.CodeAnalysis.CSharp project.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sharwell, Microsoft.CodeAnalysis.CSharp.csproj doesn't currently contain any DefineConstants. Do I need to add a new one and use if for the #if directive?

Copy link
Member

Choose a reason for hiding this comment

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

The other option is to use a fully-qualified cref attribute:

<see cref="T:Microsoft.CodeAnalysis.CSharp.Syntax.OmittedTypeArgumentSyntax"/>

Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>
@Youssef1313
Copy link
Member Author

Youssef1313 commented Sep 1, 2020

@sharwell, error CA1200: Avoid using cref tags with a prefix

Should I disable CA1200 in editorconfig? or in a ruleset file? or inline in the same file only? or a GlobalSuppressions.cs?

@sharwell
Copy link
Member

sharwell commented Sep 1, 2020

error CA1200: Avoid using cref tags with a prefix

I would disable it in just this file using a #pragma, and update the default comment to also say that the usage is required since the file is included in projects that do not have access to the syntax nodes.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 16). @dotnet/roslyn-compiler for a second review.

@333fred 333fred added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Sep 1, 2020
@jcouv jcouv self-assigned this Sep 8, 2020
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 16)

@jcouv jcouv merged commit 38c8b16 into dotnet:master Sep 8, 2020
@ghost ghost added this to the Next milestone Sep 8, 2020
@Youssef1313 Youssef1313 deleted the patch-7 branch September 8, 2020 20:12
@dibarbet dibarbet modified the milestones: Next, 16.8.P4 Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to find documentation on some enum values like PointMemberAccessExpression
7 participants