-
Notifications
You must be signed in to change notification settings - Fork 299
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
Add color options for Attributes, command arguments and loop labels. #1724
base: master
Are you sure you want to change the base?
Conversation
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 think it would be useful to have the attribute color configurable, but making it cyan by default collides with the default emphasis color, which might make it confusing when doing back/forward history searching (e.g. ctrl+r
).
For the command argument color, it may be useful, but I'm not sure if we can accurately identify only the arguments, especially when it comes to native commands.
For the loop label, I doubt whether it's worth to make its color configurable, given how often it gets used.
@lzybkr Can you please share your thoughts?
if ((token.Kind == TokenKind.Identifier | token.Kind == TokenKind.Generic) && token.TokenFlags == TokenFlags.None) | ||
{ | ||
return _options._commandArgumentColor; | ||
} |
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 not sure if this is accurate to reflect command arguments. Could the condition be true for something that is not a command argument?
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 don't think so, I've pasted a lot of code into the console to test the colors and I haven't found any unexpected results regarding command arguments missing color or affecting something that wasn't a command argument.
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 would suggest using logical or instead of bitwise or.
That said, with these changes, which tokens get the default now? I can't think of any, in which case, it seems unnecessary to introduce a new option.
I'm not opposed to adding the loop label, but obviously it's rarely used. I'm not excited with using a different color for attributes and types by default, but it seems fine to have the option. I'm also not excited by the change to command argument coloring by default, I think it shouldn't change from the current setting. If these changes are accepted, what tokens would fall under the default color? With this change, all I can think of are delimiters like parenthesis and braces. Thinking about this a little more, I think coloring command arguments not using the default color might be a surprising and annoying change for anyone who customizes the default. |
I understand not wanting to change the defaults. Into something like:
So if you run: |
PR Summary
Add 3 new color options for tokens that currently use the default token color.
More color options will make it easier to replicate themes from editors like VS code and the ISE into the normal console.
The default colors for these new tokens are:
PR Checklist
Microsoft Reviewers: Open in CodeFlow