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

Brand new TextMate grammar for providing C# syntax highlighting #1115

Merged
merged 138 commits into from
Jan 12, 2017

Conversation

DustinCampbell
Copy link
Member

@DustinCampbell DustinCampbell commented Jan 11, 2017

Fixes #101, #225, #268, #316, #674, #706, #731, #746, #782, #802, #816, #829, #830, #861, #1078, #1084, #1086, #1091, #1096, #1097, #1106, #1108

This is a large change that addresses several syntax highlighting bugs and provides a brand new TextMate grammar for C#. While this grammar will initially be checked in here, it will ultimately be migrated to https://github.com/dotnet/csharp-tmLanguage where it will live as the official C# TextMate grammar from the C# team.

The new grammar is much more robust than the one previously used and is modeled after the grammar used for TypeScript: https://github.com/microsoft/TypeScript-tmLanguage.

To review the new grammar, please see syntaxes/csharp.tmLanguage.yml.

Major changes:

  • The grammar is now authored as a YAML file which is more compact, supports comments, and allows us to split complex regular expressions across multiple lines for clarity.

  • Type names are no longer matched as a single blob of text. Instead, they are matched recursively and each identifier and piece of punctuation is classified separately:

    image

  • Generally, each production is each matched separately (though care is taken to ensure that matches begin with non-terminal). This is a big improvement over the previous grammar because future changes are more localized and matches can be more contextual).

  • ~400 unit tests to protect against future regressions.

cc @CyrusNajmabadi, @jasonmalinowski, @dpoeschl, @rchande, @mattwar, @MadsTorgersen, @jaredpar, @damieng, @alexandrudima, @jrieken, @david-driscoll, @filipw

Here's an example of how things look with the new grammar:

image

@@ -474,13 +478,11 @@
"razor"
]
},
"runtime": "node",
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't want this change

Copy link
Member Author

Choose a reason for hiding this comment

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

sigh I always forget those. Fixing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for spotting those changes! They're reverted now.

"runtimeArgs": [],
"variables": {
"pickProcess": "csharp.listProcess",
"pickRemoteProcess": "csharp.listRemoteProcess"
},
"program": "./out/src/coreclr-debug/proxy.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Or this one

@@ -1140,8 +1142,17 @@
"request": "attach",
"processId": "${command.pickProcess}"
}
]
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Or this

@damieng
Copy link

damieng commented Jan 11, 2017

Wish I'd known this was coming - spent a lot of time fixing up the existing grammar atom/language-csharp#87

Any plans to move this to the grammar-only repo at https://github.com/dotnet/csharp-tmLanguage or is it going to reside here after all?

@DustinCampbell
Copy link
Member Author

@damieng: Sorry you were working so hard on those changes! I worked on this mostly over holiday vacation time, so I apologize for not checking in with you. I suppose it's a little funny since we were likely working on our changes within a half-hour drive of one another. 😄

Yes, the new grammar will be moving to the CSharp-tmLanguage repo. For now, it's here simply for convenience and for kicking the tires while we get that repo up and running. Once it's there, this will become the official C# grammar and we can all contribute there.

@filipw
Copy link
Contributor

filipw commented Jan 11, 2017

excellent!

Fixes #101, #225, #268, #316, #674, #706, #731, #746, #782, #802, #816, #829, #830, #861, #1078, #1084, #1086, #1091, #1096, #1097, #1106, #1108

you might have broken a Github record 😄

@DustinCampbell
Copy link
Member Author

@ivanz, @Pilchie: Could I get a sign off from either of you? I realize that providing a full review is probably unrealistic, but I'd love to get this merged so we can start kicking the tires for 1.7 of C# for VS Code.

@tverboon
Copy link

Would love to use a preview release and give feedback if I encounter any problems or see something strange. Working full time in VS Code 🎉 ❤️

@Pilchie
Copy link
Member

Pilchie commented Jan 12, 2017

I won't be able to take a look until Monday. Sorry :-/

@ivanz
Copy link
Contributor

ivanz commented Jan 12, 2017

Amazing! As you say - it's massive to review. Conceptionally looks good and I am happy to approve, so it can be tested in practice :)

@DustinCampbell
Copy link
Member Author

Thanks! Merging.

@DustinCampbell DustinCampbell merged commit 4d90180 into dotnet:master Jan 12, 2017
@jlmakes
Copy link

jlmakes commented Jan 16, 2017

Epic!!

Is there a rough estimation of when these improvements will make it into a release?

@DustinCampbell
Copy link
Member Author

I'm hoping to get a new beta release out here on the site that adds this and some other fixes for an upcoming .NET Core release. As far as the official release, the 1.7 milestone is currently slated for 1/31. We'll see if we make that date or not. 😄

@jasonmeisel
Copy link

Awesome! Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants