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

Soft illegals #1839

Closed
wants to merge 2 commits into from
Closed

Conversation

brunophilipe
Copy link
Contributor

@brunophilipe brunophilipe commented Sep 20, 2018

Instead of ignoring illegals completely, using "soft illegals" will cause highlight.js to use the illegal expressions as an alternate end expression. This way, it will still refuse to highlight illegal expressions, but without bailing out completely.

Please note my experience with JavaScript is limited. Perhaps there is a better way to achieve this behavior. However I tested it and it works very well! This is interesting for my use case because I want to be able to highlight small chunks of code at a time in my code editor, which are not necessarily correct, as the user might still be composing them.


For example. This illegal multi-line string is currently highlighted when using "ignore_illegals":

char *string = "foo\
bash
baz";

screen shot 2018-09-20 at 01 25 57


Using soft illegals, the string expression is highlighted up to the "bash" word, after which the expression ends. The resulting highlight is better than not having any highlight at all, and actually matches exactly what Xcode highlights for the same expression:

screen shot 2018-09-20 at 01 22 52

In Xcode:

screen shot 2018-09-20 at 01 22 42

Cheers!

Instead of ignoring illegals completely, soft_illegals will use the illegal expressions as an alternate end expression.
Copy link
Contributor

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Interesting suggestion. Some API modifications while I think this through a bit.

*/
function highlight(name, value, ignore_illegals, continuation) {
function highlight(name, value, soft_illegals, continuation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

so, instead of changing the function signature in this manner, I suggest you instead change it to accept an an object as the last argument:

function highlight(name, value, ignore_illegals, continuation, options) {
}

Then pass the options to compileLanguage:

compileLanguage(language, options);

And then check the options.soft_illegals... I would also change soft_illegals to treat_illegals_as: "soft", that way, it's more extensible in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Definitely agree.

Copy link
Member

@joshgoebel joshgoebel left a comment

Choose a reason for hiding this comment

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

Also we really should have at least one test for this to show the behavior both ways... and how does this intent to interact with ignore_illegals now? (since it's a sep option)

I assume this would be paired with ignore_illegals: false, right?

  • ignore_illegals: false, treat_illegals_as: "soft": illegal ends the match
  • ignore_illegals: false, treat_illegals_as: "hard"/null: illegal raises

And the behavior with ignore_illegals true is that it just blows by them and keeps looking for the final end match, yes?

@@ -278,7 +278,12 @@ https://highlightjs.org/
mode.end = mode.begin;
if (!mode.end && !mode.endsWithParent)
mode.end = /\B|\b/;
if (mode.end)
if (soft_illegals && mode.end && mode.illegal) {
var endSource = (mode.end instanceof RegExp ? mode.end.source : mode.end);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this why we have the reStr helper?

*/
function highlight(name, value, ignore_illegals, continuation) {
function highlight(name, value, soft_illegals, continuation) {
Copy link
Member

Choose a reason for hiding this comment

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

Definitely agree.

@joshgoebel
Copy link
Member

joshgoebel commented Oct 11, 2019

Having two different settings does seem messy... I wonder if it wouldn't be clearer to change the parameter to "handle_illegals": ["ignore", "end_match", "error"]... we could grandfather in true and false for ignore/error...

@joshgoebel joshgoebel added the enhancement An enhancement or new feature label Oct 11, 2019
@joshgoebel
Copy link
Member

This needs to be held until after #2277 in which case we can just come up with the perfect clear name and then add it as a boolean flag.

@joshgoebel
Copy link
Member

Actually this code base would have to be entirely reworked, so I'm closing this PR for now. It would likely need to interface with processLexeme (at run-time) not hack the modes at compile time. We only compile modes a single time, so the mode itself needs to be static (post-compile) and cannot be dependent upon run-time config constraints, like it is in this patch.

for my use case because I want to be able to highlight small chunks of code at a time in my code editor

Also, just FYI this is not really a supported use-case. Of course you're free to do whatever you want with the library, but I'm hesitant to add features just to support a use-case we actually don't encourage or support.

I'd suggest we first consider how to possibly solve this with a plugin rather than adding it to the core highlighter. Perhaps an on:Illegal hook/callback of some kind that plugins could tap into... OR I've also been considering mode compile callbacks... so a plugin could perhaps mess with the mode compilation and turn illegal into an end variant (like this PR does)... or course this would have the same caveats I mention above, but that might not be a concern for the users of the plugin.

If we want to continue the discussion I'd suggest opening a new issue thread and pointing to this PR for context.

@joshgoebel joshgoebel closed this Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature on hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants