Massive overhaul of grammar #87

Merged
merged 36 commits into from Dec 30, 2016

Conversation

Projects
None yet
4 participants
@damieng
Contributor

damieng commented Dec 26, 2016

Ideally this would have been smaller changes but the grammar had many many issues.

This adds support to or fixes highlighting for:

  • Generic methods
  • Generic types
  • Type constraints
  • Extern fields
  • Nullable/pointer built-in types
  • Lambda method syntax (C# 6)
  • Method bodies
  • Type recognition on cast/as/in/sizeof/typeof
  • Interface names (convention based on I[A-Z] in absence of type engine)
  • Improvements in newline handling (allowing more, not requiring any)

Ideally would have more work done on newlines and comments being allowed everywhere but that's an ongoing issue.

Here's what code can now look like using a VS-style theme I put together;

image

Still remaining before merge:

  1. Fix specs
  2. Get basic types parsing in method bodies

damieng added some commits Dec 17, 2016

damieng added some commits Dec 28, 2016

@damieng damieng merged commit f013fd7 into master Dec 30, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@damieng damieng deleted the dg-generic-parameters branch Dec 30, 2016

@heyimalex

This comment has been minimized.

Show comment
Hide comment
@heyimalex

heyimalex Jan 4, 2017

Should this be in a patterns array? I was looking into how tmLanguage grammars are structured and this is the only one out of the ~270 I've looked at that has match as a key in the capture group object. It seems ambiguous because what gets the scope name, the originally captured text or the sub matched text?

Should this be in a patterns array? I was looking into how tmLanguage grammars are structured and this is the only one out of the ~270 I've looked at that has match as a key in the capture group object. It seems ambiguous because what gets the scope name, the originally captured text or the sub matched text?

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Jan 4, 2017

Contributor

I think the best option here is to remove the match entirely and make its parent not be deferred so it's not duplicating logic.

Contributor

damieng replied Jan 4, 2017

I think the best option here is to remove the match entirely and make its parent not be deferred so it's not duplicating logic.

@tomchkk

This comment has been minimized.

Show comment
Hide comment
@tomchkk

tomchkk Jan 7, 2017

Contributor

@damieng Why is 'meta.class.cs' no longer part of the required scope here?

Contributor

tomchkk commented on spec/grammar-spec.coffee in 4bf9d94 Jan 7, 2017

@damieng Why is 'meta.class.cs' no longer part of the required scope here?

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Jan 7, 2017

Contributor

It was removed as it messes up a whole bunch of syntax highlighting in various themes. It also doesn't make much sense as you can have classes in classes etc and so it starts nesting.

Are there some themes/scenarios that were actually using it?

Contributor

damieng replied Jan 7, 2017

It was removed as it messes up a whole bunch of syntax highlighting in various themes. It also doesn't make much sense as you can have classes in classes etc and so it starts nesting.

Are there some themes/scenarios that were actually using it?

This comment has been minimized.

Show comment
Hide comment
@tomchkk

tomchkk Jan 8, 2017

Contributor

I'm using a fork of this grammar for something completely different. But it seemed to me that the very essence of scopes is that they are nested.

I can see why having more generalised scopes might not usually matter for syntax-highlighting, but say you wanted – for whatever reason – to highlight nested classes differently to non-nested classes, the scope wouldn't give you that level of detail anymore. You'd only know that a nested class is a class.

Likewise, if a scope doesn't tell you that a method-call is within a class, for instance, then as far as your syntax highlighting is concerned that method-call could be anywhere in the source.

Contributor

tomchkk replied Jan 8, 2017

I'm using a fork of this grammar for something completely different. But it seemed to me that the very essence of scopes is that they are nested.

I can see why having more generalised scopes might not usually matter for syntax-highlighting, but say you wanted – for whatever reason – to highlight nested classes differently to non-nested classes, the scope wouldn't give you that level of detail anymore. You'd only know that a nested class is a class.

Likewise, if a scope doesn't tell you that a method-call is within a class, for instance, then as far as your syntax highlighting is concerned that method-call could be anywhere in the source.

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Jan 8, 2017

Contributor

The problem from a syntax highlighting point of view is it's very hard to use the meta classes without messing things up because of precedence rules. Some of the themes I tried had overridden meta.class which basically wiped out all the syntax highlighting.

Contributor

damieng replied Jan 8, 2017

The problem from a syntax highlighting point of view is it's very hard to use the meta classes without messing things up because of precedence rules. Some of the themes I tried had overridden meta.class which basically wiped out all the syntax highlighting.

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jan 8, 2017

Member

That's really unfortunate that those themes try to style meta.class. Meta scopes are meant to be used for autocomplete, not styling (as long as autocomplete could theoretically benefit from those scopes, unlike this).

Overall, I agree that meta.class should exist, but if themes are attempting to style it, then I guess this is the better short-term solution.

Member

50Wliu replied Jan 8, 2017

That's really unfortunate that those themes try to style meta.class. Meta scopes are meant to be used for autocomplete, not styling (as long as autocomplete could theoretically benefit from those scopes, unlike this).

Overall, I agree that meta.class should exist, but if themes are attempting to style it, then I guess this is the better short-term solution.

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Jan 8, 2017

Contributor
Contributor

damieng replied Jan 8, 2017

@damieng damieng referenced this pull request in OmniSharp/omnisharp-vscode Jan 11, 2017

Merged

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

@arcticicestudio arcticicestudio referenced this pull request in arcticicestudio/nord-atom-syntax Feb 25, 2017

Closed

C# grammar overhaul adaptation #38

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment