dotnet / vscode-csharp

Official C# support for Visual Studio Code
MIT License
2.87k stars 676 forks source link

Extract C# grammar into own repo to avoid effort duplication between Atom and VSCode? #877

Closed ivanz closed 7 years ago

ivanz commented 8 years ago

Appears to me that there is some ongoing duplicated effort with the C# textmate grammar in VSCode (gitlog) and Atom (gitlog).

Seems like the only real difference between the two is JSON vs CSON, so why not extract the VSCode textmate C# grammar and test harness setup into a separate repo in OmniSharp, port any fixes from Atom and then share the same grammar across the two extensions (outping CSON, etc)?

What do you guys think?

/cc @DustinCampbell /cc @david-driscoll /cc @damieng

DustinCampbell commented 8 years ago

That's a possibility, though we're also pulling the C# grammar from here into Visual Studio itself. Atom could pull from here as well. If we want to create a separate repo, I would probably create it in the dotnet organization.

/cc @Pilchie

ivanz commented 7 years ago

Having it in the dotnet org makes sense. I don't really mind. Main thing is to avoid duplicated work between VSCode, Atom and potentially others given that there have been substantial improvements to the C# grammar (imho) and its test coverage in the VSCode repo already.

What's the next best step here?

damieng commented 7 years ago

I'm up for sharing a common grammar file for C# if we can. Ideally the repo would be just enough for the grammar and a platform-agnostic test suite. That way we might be able to include it as a submodule or some kind of other similar mechanism to wrap it with each products necessary additional files.

The other issue to sort out would be the license. The one on this repo doesn't include any of the previous licencing from atom/language-csharp which includes GitHub and TextMates licence info in https://github.com/atom/language-csharp/blob/master/LICENSE.md

ivanz commented 7 years ago

@damieng The mentioned license info is in: https://github.com/OmniSharp/omnisharp-vscode/blob/master/ThirdPartyNotices.txt

I've extracted the vscode grammar and tests in a repo here: https://github.com/ivanz/csharp-textmate-grammar

It currently takes the VSCODE json and converts it to Atom cson in the grammars/ folder after npm i && gulp. It's an npm package where the grammars directory is published only, but I haven't published it yet.

Travis CI build here: https://travis-ci.org/ivanz/csharp-textmate-grammar

Thoughts? I can try and see what fixes and tests can be ported from Atom's repo if we are happy with this approach? Happy to transfer repo to the dotnet org or wherever is best too.

c:\code\csharp-textmate-grammar>dir grammars
12/12/2016  21:49            16,626 csharp.cson
12/12/2016  21:49            20,698 csharp.json
DustinCampbell commented 7 years ago

Note that Visual Studio also picks up this grammar too. I'm all for having this someplace a bit more official. Let me do some digging to figure out the best place to put it.

DustinCampbell commented 7 years ago

At least, figure out what it takes to get a repo in the dotnet organization. I actually don't think it's that big of a deal. Just need to ping the right people.

ivanz commented 7 years ago

Thanks dude. Sorry if I am causing you a bit of a pain with this.

ivanz commented 7 years ago

BTW for whoever reads this - do not get fooled by the "vscode" in the "vscode-textmate" package used the repo - it's not vscode specific at all nor does it pull any vscode dependencies (it's simply a textmate grammar parser+tokenizer)

DustinCampbell commented 7 years ago

Looping in @martinwoodward from the .NET foundation.

Martin -- we're hoping to move the syntax grammar used for C# colorization someplace where it can be more easily shared between VS Code, Visual Studio and Atom. Is this something that we could pull into the .NET Foundation? I think it'd also be important to pull it into the "dotnet" organization on GitHub since it'd essentially be the official place for the C# syntax grammar shared between several editors. What would that entail? Could you help us get that done or loop us in with the right folks to make it happen?

martinwoodward commented 7 years ago

I think that's a great idea - what do you want the repo called? I'll get it created and make you an admin @DustinCampbell and then you can add the other committers who need access. Note committers must sign the .NET Foundation CLA and agree to the .NET Foundation Code of Conduct - but as long as all changes go through PR's we'll get the CLABot to check that for you.

jskeet commented 7 years ago

To be clear, would this only be enough to provide syntax coloring? I would imagine that's considerably less involved than the full C# grammar required by the language specification, which has various interesting nuances that editors probably don't care about.

DustinCampbell commented 7 years ago

@jskeet, that's correct. It's only regex's anyway, so the it's a poor man's approximation the syntax grammar. FWIW, we have all of the information we'd need to colorize properly (identically to Visual Studio in fact) from OmniSharp and Roslyn, but VS Code does not yet provide an API that would allow us to take advantage of this information. (Hopefully we'll get https://github.com/Microsoft/vscode/issues/585 someday.)

Side note: It's Visual Studio 2017 that uses this grammar, and that's only to use when C# isn't installed. It uses a textmate grammar as a fallback to colorize files when the expected language isn't installed.

david-driscoll commented 7 years ago

In the past I've tried to insert grammar rules into Atom, but it has generally always fallen apart in many ways (bugs, performance, look and feel).

For VS Code it's entirely possible that we could insert icons instead of colors, but that wouldn't always be the best result.

DustinCampbell commented 7 years ago

@martinwoodward: I think the name that @ivanz is probably the right one: "csharp-textmate-grammar"

martinwoodward commented 7 years ago

Sounds good - I'll get this created. We can always change the name later if we have to

martinwoodward commented 7 years ago

Created https://github.com/dotnet/csharp-textmate-grammar and made you admin @DustinCampbell

DustinCampbell commented 7 years ago

You rock -- thanks @martinwoodward!

DustinCampbell commented 7 years ago

Do I need to do anything to set up the CLA stuff?

martinwoodward commented 7 years ago

I'll take care of it - Working on that now

DustinCampbell commented 7 years ago

Cool. Initially, the commiters are folks who have already signed the CLA. E.g. @ivanz and potentially @damieng

DustinCampbell commented 7 years ago

At this point, I think this is done.