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

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

Closed
ivanz opened this issue Nov 5, 2016 · 21 comments
Assignees
Milestone

Comments

@ivanz
Copy link
Contributor

ivanz commented Nov 5, 2016

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
Copy link
Member

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
Copy link
Contributor Author

ivanz commented Dec 11, 2016

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
Copy link

damieng commented Dec 11, 2016

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
Copy link
Contributor Author

ivanz commented Dec 12, 2016

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

ivanz commented Dec 12, 2016

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

@ivanz
Copy link
Contributor Author

ivanz commented Dec 12, 2016

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
Copy link
Member

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
Copy link
Member

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
Copy link

jskeet commented Dec 19, 2016

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
Copy link
Member

@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 microsoft/vscode#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
Copy link
Contributor

david-driscoll commented Dec 19, 2016

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
Copy link
Member

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

@martinwoodward
Copy link
Member

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

@martinwoodward
Copy link
Member

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

@DustinCampbell
Copy link
Member

You rock -- thanks @martinwoodward!

@DustinCampbell
Copy link
Member

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

@martinwoodward
Copy link
Member

martinwoodward commented Dec 20, 2016

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

@DustinCampbell
Copy link
Member

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

@DustinCampbell
Copy link
Member

At this point, I think this is done.

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

No branches or pull requests

6 participants