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

Colorize 'args' in top-level program #59574

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Colorize 'args' in top-level program #59574

merged 1 commit into from
Feb 16, 2022

Conversation

CyrusNajmabadi
Copy link
Member

Fixes #59569

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner February 16, 2022 00:03
dibarbet
dibarbet reviewed Feb 16, 2022
View reviewed changes
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

could you post a small screenshot of what the before / after looks like?

JoeRobich
JoeRobich approved these changes Feb 16, 2022
View reviewed changes
Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

At what point do we extract all the contextual keyword classification into a separate TryClassifyContextualKeyword function for readability sake?

@davidwengier
Copy link
Contributor

Screenshot would be good..

I wonder if we'll get feedback like "how come the word args is sometimes blue?"

sharwell
sharwell approved these changes Feb 16, 2022
View reviewed changes
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 behavior here is consistent with our behavior for value in setters and event accessors.

Youssef1313
Youssef1313 reviewed Feb 16, 2022
View reviewed changes
src/Workspaces/CSharp/Portable/Classification/SyntaxClassification/NameSyntaxClassifier.cs
Comment on lines +178 to +179
if (name is IdentifierNameSyntax { Identifier.Text: "args" } &&
symbol is IParameterSymbol { ContainingSymbol: IMethodSymbol { Name: WellKnownMemberNames.TopLevelStatementsEntryPointMethodName } })
Copy link
Member

Choose a reason for hiding this comment

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

Is the first check necessary? I think it can be moved as an assert inside the condition. It should be okay either way.

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 thought about that, but i like the code being explicit about the precise case it is handling.

@CyrusNajmabadi
Copy link
Member Author

Looks like a keyword :)

image

@CyrusNajmabadi CyrusNajmabadi merged commit c97297e into dotnet:main Feb 16, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the colorizeArgs branch February 16, 2022 23:40
@ghost ghost added this to the Next milestone Feb 16, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dibarbet dibarbet dibarbet left review comments

@Youssef1313 Youssef1313 Youssef1313 left review comments

@JoeRobich JoeRobich JoeRobich approved these changes

@sharwell sharwell sharwell approved these changes

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
17.2.P2
Development

Successfully merging this pull request may close these issues.

args in top-level program should be colored as a contextual keyword.
7 participants
@CyrusNajmabadi @davidwengier @JoeRobich @sharwell @dibarbet @Youssef1313 @RikkiGibson