search

mjbvz / vscode-fenced-code-block-grammar-injection-example

Example of injecting a new grammar into VSCode's builtin markdown syntax highlighting for fenced code blocks
MIT License
98 stars 26 forks source link

Is this repo out of date? #3

Closed kachkaev closed 6 years ago

kachkaev commented 6 years ago

Thanks for putting together this example @mjbvz! I found a few references to your code in the issues of Microsoft/vscode repo and could use it as guidance to implement https://github.com/Krzysztof-Cieslak/vscode-elm/pull/186.

I noticed a small issue when replicating your code ‘as is’, but am not sure if that's because of the recent changes in VSCode or simply due to the lack of my experience (I'm using VSCode just since last Wednesday). Here's where the main problem was: https://github.com/mjbvz/vscode-fenced-code-block-grammar-injection-example/blob/f341990d411fe1e3f2243cf9ca608f4eaa168c3b/syntaxes/codeblock.json#L10-L12

After replacing superjs with elm, ```elm blocks began to highlight, however, the same also became true for ```elm42, ```elmABCD and so on. Besides, in some cases the code was ‘spilling’ out of the fenced block, thus highlighting the following markdown incorrectly. This situation looks related to what's been reported in https://github.com/mjbvz/vscode-fenced-code-block-grammar-injection-example/issues/2.

Could it be that your example needs updating by making codeblock.js a bit more similar to markdown.tmLanguage.json in the core repo? If so, it'd be great if you could show the up-to-date way of injecting the grammars!

Perhaps, another thing that'd be great to improve is README. It could contain a new section with steps to follow in order to get the result. These could be as simple as copy this bit from this file, ensure this is replaced with this, etc. I'm sure that the newcomers like me will appreciate such a guidance!

mjbvz commented 6 years ago

@kachkaev You can make the begin rule more specific if you don't want it to match elm42 or similar. I'll update the example to do this since this is usually the desired behavior. However the overall injection logic seems to work fine even without this change:

screen shot 2018-02-12 at 6 28 49 pm

What problem were you running into?

kachkaev commented 6 years ago

There were two problems despite that the code highlighted in general:

  1. ```superjs42 as a false positive

  2. syntax spilling beyond ``` in certain cases (could this be an uclosed string template in the case of superjs?)

All blocks in vscode core are declared differently and do not have the problems above. This experience and #2 suggest that it is reasonable to change this example.

The issue is still open IMHO :–)

mjbvz commented 6 years ago

Fixed. See latest version in master

kachkaev commented 6 years ago

Thank you!