-
-
Notifications
You must be signed in to change notification settings - Fork 382
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
switch syntax highlighting to Linguist w/ auto-detection #28
Conversation
Removing the outer fenced block since I can't figure out how to escape the inner fenced block :/
``` ruby | ||
some_code(:first) | ||
``` | ||
result = pipeline.call <<-MDOWN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use <
here? Does it render incorrectly on github otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not but just to be on the safe side. <pre>
allows HTML tags so anything that's not a tag but starts with <
might confuse the parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
I love the thorough documentation documentation you added with the filter. Since this is still a pre-1.0 release and the docs previously lied about having this feature implemented, let's leave auto-detection on by default. I'm 👍 for this pull (couple minor things commented inline). I'll merge it by Wednesday unless other people have more refinements. |
<pre> tags without a "lang" attribute are now auto-detected for language among the popular languages that Linguist can recognize. This behavior is default and can be turned off with `:detect_syntax => false`
Since everything now goes through Linguist, even when the language is specified explicitly, I was wondering does it support all the languages and their aliases as Pygments did. I've summarized the results here. Turns out there are a lot of languages/aliases that Pygments supported and which aren't accessible anymore through Linguist. Full list is in the gist above, but here are some of which caught my eye:
We could go straight to Pygments if Linguist didn't find anything, but that means adding a separate code path and a direct dependency to Pygments.rb. |
It's just the aliases that aren't being properly recognized right? Does the actual syntax highlighting still happen if you specify the correct alias? If it's a problem with what aliases Linguist supports, I'd rather fix that upstream rather than test for it in this gem. Out of curiosity, what command are you using to generate that report of missing aliases? |
Oh I've already done as github-linguist/linguist#307 Most of those are just aliases/file extensions that Pygments recognizes and Linguist doesn't, but for instance "erb" is among the languages simply not supported through Linguist (but added in my patch). I'm just saying that pulling this change will reduce the number of previously recognized names. GitHub.com uses html-pipeline, right?, and when they update to this version some previously highlighted code blocks may not be highlighted anymore. I have added the script to the gist |
I'll double check on these changes and also on #26 to make sure there's no legacy reason for why the code didn't directly use Pygments. |
Closing out some old issues. Leaving this until we can do another pull request which would be backwards compatible with all the languages. |
Auto-detection is on by default, but can be turned off via a context
option. Note that auto-detection is happy to classify even a simple run
of text as a programming language:
We might consider turning it off by default to keep backwards
compatibility and reduce surprising behavior. With fenced code blocks in
GitHub Markdown, it's easy to tag code blocks with a "lang" attribute.