asciidoctor / atom-asciidoc-preview

⚛ AsciiDoc preview for the Atom editor.
https://atom.io/packages/asciidoc-preview
MIT License
141 stars 42 forks source link

Allow highlight for PHP even without <?php #265

Closed shemgp closed 6 years ago

shemgp commented 6 years ago

Description

Allow highlight for PHP even without starting <?php.

Syntax example

[source,php]
----
echo "highlight me please";
----

Screenshots

screenshot from 2018-04-10 22-03-00

shemgp commented 6 years ago

Any hint on how to improve this so you'll accept it? pygments highlighter already has an exception for this (https://github.com/asciidoctor/asciidoctor/blob/888c96f6e30464ec49bc48b28dfa20c766e3b427/lib/asciidoctor/substitutors.rb#L1447) Thanks for your consideration.

mojavelinux commented 6 years ago

I agree the highlighter should support PHP without the opening <?php directive. However, I think this problem needs to be solved upstream in highlights package (specifically the php language support package) instead using a workaround in this package.

Can you file the issue here: https://github.com/atom/language-php?

shemgp commented 6 years ago

Forgive my ignorance, but looking at the code I can't seem to find where you call highlightjs's hljs.highlightBlock(block);. If I can find that, then maybe it'll be easy to make highlighjs highlight PHP code even without <?php. (https://jsfiddle.net/xpvt214o/98838/)?

mojavelinux commented 6 years ago

This add-on does not use highlight.js, it uses highlights, which is the Atom syntax highlighter. (I recognize the similarity of names is confusing).

The best way to proceed is to file an issue with https://github.com/atom/language-php and see what information we get back. That will help us understand where this change belongs.

The reason I think this is an upstream issue is that the following doesn't work in the preview for a Markdown document either:

```php
echo "highlight me please";
```

I think Atom should have some way to enable inline mode for PHP highlighting. Then we just have to enable it selectively.

shemgp commented 6 years ago

File the bug report already. Actually part of the reason I thought that highlights.js is being used is because you have a templates\default.html in your code which uses highlight.js. Is that being used?

mojavelinux commented 6 years ago

For reference, here's the upstream issue: https://github.com/atom/language-php/issues/328

mojavelinux commented 6 years ago

The templates file is used for exporting I believe. It's not used by the preview itself.

shemgp commented 6 years ago

Looking at https://github.com/atom/language-php/issues/182 it seems upstream supports highlighting PHP without <?php already. Here's a proof of concept patch that uses that feature.

ldez commented 6 years ago

@shemgp I moved the code to highlights-helper.coffee.

I don't really like to have a specific language condition but if the code is small and isolated I think it's reasonable.

mojavelinux commented 6 years ago

I agree this is a reasonable change and the right way to use highlights.

What I realize, though, is that the "mixed" option should probably make its way to the generated HTML so we can enable this conditionally based on whether that option is present. We could add a special class to the HTML in core. See https://github.com/asciidoctor/asciidoctor/pull/2015#issuecomment-280275350

For now, though, the only way is to detect whether <?php is present.