Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

C# highlighting improvements #1795

Merged

Conversation

Philipp-M
Copy link
Contributor

Adds more queries for better highlighting:

before:
helix-csharp-highlight-before

after:
helix-csharp-highlight-improvement

@the-mikedavis the-mikedavis self-requested a review March 11, 2022 19:27
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I haven't dug into this fully yet but it's looking like a great improvement! There are some highlights I've seen that I think would be good to include here as well if you like them:

; this list already exists and we should be able to just add "static" in
[
  ; ..
  "static"
  ; ..
] @keyword

highlights a block like so:

using Status = Google.Rpc.Status;
using static EventStore.Client.Streams.BatchAppendReq.Types;
using static EventStore.Client.Streams.BatchAppendReq.Types.Options;
using OperationResult = EventStore.Core.Messages.OperationResult;
((identifier) @comment.unused
 (#eq? @comment.unused "_"))

we've used in other languages that support pattern matching to mark it as an unused pattern.

For example in an expression like so:

Task.Delay(timeout, cancellationToken).ContinueWith(_ => onTimeout(), cancellationToken);

@Philipp-M Philipp-M force-pushed the csharp-highlights-improvement branch from af3b264 to 4a01edb Compare March 12, 2022 12:21
@Philipp-M
Copy link
Contributor Author

Thanks for the input, I've adapted your changes

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

one more highlight I found looking at some example code:

[
  ; .. L148
  ">"
  ">="
] @operator

With that I think this will be ready to merge 👍

@Philipp-M Philipp-M force-pushed the csharp-highlights-improvement branch from 4a01edb to 724ead1 Compare March 12, 2022 17:47
@Philipp-M
Copy link
Contributor Author

I missed that there were operators that aren't covered yet, I checked this: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/#operator-precedence

And I think I covered now every operator in one way or the other (something like "?." was not possible because it's already covered by "." and "?" I guess?)

@the-mikedavis
Copy link
Member

It's a bit confusing what can be written in strings with the query DSL: you can only write literal tokens that show up also as strings in the grammar (for example, these tokens). I don't see ?. in grammar.js - there must be some rule that parses the ? and . as separate tokens.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Great work! This is a big improvement 💪

@the-mikedavis the-mikedavis merged commit 6fdf5d0 into helix-editor:master Mar 12, 2022
@Philipp-M Philipp-M deleted the csharp-highlights-improvement branch March 12, 2022 18:46
the-mikedavis pushed a commit to the-mikedavis/helix that referenced this pull request Mar 12, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants