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

[Markdown] add syntax highlighting in code fences for a few languages #1274

Merged
merged 14 commits into from
Apr 15, 2018

Conversation

keith-hall
Copy link
Collaborator

This PR uses the new embed action to add syntax highlighting inside Markdown code fences for XML, SQL, Python and Graphviz.

Copy link
Collaborator

@FichteFoll FichteFoll left a comment

Choose a reason for hiding this comment

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

What about other frequently used languages like Java, JavaScript, C (and variants) etc.?

- match: |-
(?x)
{{fenced_code_block_start}}
((?i:python))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also add the 'py' variant, which I use all the time
😉

embed_scope: markup.raw.code-fence.xml.markdown
escape: '^[ ]{,3}\1\s*$'
escape_captures:
0: markup.raw.code-fence.xml.markdown punctuation.definition.raw.code-fence.end.markdown
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only the fences should get the punctuation scope, not any whitespace surrounding it.

@keith-hall
Copy link
Collaborator Author

Adjustments made - I deliberately didn't add too many languages to start with in case tweaks were needed, as unfortunately variables are not supported in the escape pattern, and I was lazy to make lots of syntax tests... About things like C, C++, Obj-C - I wonder if there is some canonical reference somewhere of what the "language identifier" in the info string should be?

- match: |-
(?x)
{{fenced_code_block_start}}
((?i:javascript))
Copy link
Collaborator

@FichteFoll FichteFoll Nov 3, 2017

Choose a reason for hiding this comment

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

js also

Disregard, github was only showing me one of the two commits.

@FichteFoll
Copy link
Collaborator

Following github's GFM docs I found this: https://github.com/github/linguist/blob/master/lib/linguist/languages.yml. Looks like it's a case-insensitive match of the name and also supports aliases, although Python doesn't have a py alias 🤔

@wbond
Copy link
Member

wbond commented Nov 6, 2017

I would expect shell is probably one of the most popular fenced syntaxes. According Tiobe, things like C, PHP, Ruby, Obj-C, and Go would probably be higher on the priority list than Rust.

@randy3k
Copy link
Contributor

randy3k commented Nov 30, 2017

I vote for the R syntax. (According Tiobe, R scores higher than GO)

@keith-hall
Copy link
Collaborator Author

I vote for embed to be changed to work more like include in that missing syntaxes are just warnings, so I can keep languages I will never use in my ignored_packages list without a load of errors appearing and the syntax highlighting not working at all, that's the main thing stopping me from adding more languages atm, apart from the extra syntax test and maintenance headache

@wbond
Copy link
Member

wbond commented Nov 30, 2017

@keith-hall That is an interesting idea. Make embedding sort of optional. The escape would still work, but if the embedded syntax isn't present, just print a warning saying it couldn't be found.

@keith-hall
Copy link
Collaborator Author

It would then potentially be possible (though I'm not sure if we want to) to add code fence support for some popular third party syntaxes too :)

@wbond
Copy link
Member

wbond commented Nov 30, 2017

Well, if it printed a warning when they were missing, that would cause the CI tests to fail

@keith-hall
Copy link
Collaborator Author

Added C, C++, Go, ObjC, ObjC++, R, Ruby

@michaelblyons
Copy link
Collaborator

Crosslinking to #266, where code fences and strikethrough are the last two issues.

@rwols
Copy link
Contributor

rwols commented Jan 13, 2018

This won't work correctly until sublimehq/sublime_text#1062 is fixed. For example, comments, line-continuations and character escapes are not rendered for Bash.

@wbond
Copy link
Member

wbond commented Feb 9, 2018

I spent a bit of time on sublimehq/sublime_text#1062 before 3157 went out, but wasn't able to come up with an implementation that didn't break existing default syntaxes in funny ways.

I think this is largely due to implementation details, but also this would end up being a change in behavior, so I may just end up documenting the edge case.

((?i:xml))
\s*$ # any whitespace until EOL
captures:
2: markup.raw.code-fence.xml.markdown-gfm punctuation.definition.raw.code-fence.begin.markdown
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think these captures should be getting the markup.raw scope. They aren't part of the "raw" segment since they are not rendered verbatim. (Also, I have dimmed background for markup.raw and it would be nice if that started on the first line of the raw block.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

And the escape_captures for 0 should be removed.

1: punctuation.definition.raw.code-fence.begin.markdown
2: constant.other.language-name.markdown
2: punctuation.definition.raw.code-fence.begin.markdown
3: constant.other.language-name.markdown
push:
- meta_scope: markup.raw.code-fence.markdown-gfm
Copy link
Collaborator

@FichteFoll FichteFoll Mar 21, 2018

Choose a reason for hiding this comment

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

For the same reason, this should be meta_content_scope. And if it would otherwise match on the newline, the match probably needs to include \n also, but I'm not sure about that.

@deathaxe
Copy link
Collaborator

How about code fence highlighting for JSON as a subset of JavaScript?

@keith-hall
Copy link
Collaborator Author

@rwols can you give some examples of some Bash code that doesn't highlight correctly in the Markdown code fence please - I've added some test assertions for comments and character escapes and it seems to be working in build 3161, but maybe I'm missing something?

@rwols
Copy link
Contributor

rwols commented Apr 10, 2018

I checked out the STealthy-and-haSTy:markdown_codefences locally and it indeed seems to work fine on 3161. So I take back what I said :)

@keith-hall
Copy link
Collaborator Author

maybe let's get this merged then 😃 🎆

@wbond wbond merged commit 1d5ea32 into sublimehq:master Apr 15, 2018
@randy3k
Copy link
Contributor

randy3k commented Sep 8, 2018

@keith-hall
I just noticed that this PR broke the regex compatibility because of the use of backreference.
https://github.com/sublimehq/Packages/pull/1274/files#diff-bc7dff9190deda9f015e7ef8a4f3ceebR108

@FichteFoll
Copy link
Collaborator

Is this variable perhaps only used in a pop pattern? If it is, it's not a problem, but checking from mobile isn't worthwhile.

@keith-hall
Copy link
Collaborator Author

keith-hall commented Sep 8, 2018

It is indeed only used from pop and escape, but ST is indeed marking it as incompatible for some reason. It's referring directly to the variable so I'd guess its a bug / overzealous warning in ST.

Packages/Markdown/Markdown.sublime-syntax:123:11: Back references are unsupported
FAILED: 1 pattern in "Packages/Markdown/Markdown.sublime-syntax" are incompatible with the new regex engine
[Finished]

(at the current state of the master branch, this is

\2 # the backtick/tilde combination that opened the code fence
)

EDIT 2018-10-29: I have logged this minor problem with the incorrect warning here: sublimehq/sublime_text#2465

@randy3k
Copy link
Contributor

randy3k commented Sep 8, 2018

R Markdown is based on the built in Markdown syntax and I recently received an issue and that seems to be related to this bug. Particular, that is resolved by reverting this PR.

deathaxe pushed a commit to deathaxe/sublime-packages that referenced this pull request Jun 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants