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

(shell) Support multiline commands #2861

Merged

Conversation

Mogztter
Copy link
Contributor

@Mogztter Mogztter commented Nov 13, 2020

resolve #2860

Changes

Add support for multi-line commands using the line continuation \.

Checklist

  • Added markup tests
  • Updated the changelog at CHANGES.md
  • Added myself to AUTHORS.txt, under Contributors

@Mogztter Mogztter force-pushed the issue-2860-command-line-continuation branch from 036b4b5 to ccaa0fe Compare Nov 13, 2020
@Mogztter
Copy link
Contributor Author

@Mogztter Mogztter commented Nov 14, 2020

end: '(?=[^\\\\]\\s*$)', subLanguage: 'bash'

Does that fix the test?

I think you have the right idea but this regular expression does not work. It actually excludes the last character.
The following:

$ /bin/sh

now produces:

<span class="hljs-meta">$</span><span class="bash"> /bin/s</span>h

(notice the letter "h" is outside the "bash" span)

@Mogztter
Copy link
Contributor Author

@Mogztter Mogztter commented Nov 14, 2020

@joshgoebel I think we need to use a negative lookbehind. Basically the line must not end with a line continuation, so the end of a line must not be preceded by zero or more spaces and the character "\".
Unfortunately, lookbehind assertions are not supported in a few browsers: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp#Browser_compatibility

I'm not sure how it can be rewritten without a lookbehind because the end position should always be the end of the line ($) unless there's a line continuation just before the end of a line. That's by definition a lookbehind assertion.

@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Nov 14, 2020

I think we need to use a negative lookbehind.

Yep, not allowed as you pointed out because Safari still lags.


So let's include the last character and use the look-ahead to find the end of line.

end: /[^\\](?=\s*$)/,

Thoughts? I pushed my changes.

@joshgoebel joshgoebel changed the title Add support for multiline command (shell) Support multiline commands Nov 14, 2020
@Mogztter
Copy link
Contributor Author

@Mogztter Mogztter commented Nov 15, 2020

Thoughts? I pushed my changes.

Oh nice, if it works then 👍
I will add a few tests to make sure that it's working as expected (and also rebase/resolve the conflict) then we are good to go!

EDIT: I can confirm that this is working great 🎉

@Mogztter Mogztter force-pushed the issue-2860-command-line-continuation branch 5 times, most recently from eb32176 to f0afb7e Compare Nov 15, 2020
@Mogztter Mogztter force-pushed the issue-2860-command-line-continuation branch from f0afb7e to 9ba8984 Compare Nov 15, 2020
@Mogztter Mogztter requested a review from joshgoebel Nov 15, 2020
@joshgoebel joshgoebel merged commit c36d5ae into highlightjs:master Nov 15, 2020
11 checks passed
@Mogztter Mogztter deleted the issue-2860-command-line-continuation branch Nov 15, 2020
@@ -32,6 +23,7 @@ New Languages:

Language Improvements:

- enh(shell) Add support for multiline commands with line continuation `\` (#2861) [Guillaume Grossetie][]
Copy link
Contributor Author

@Mogztter Mogztter Nov 15, 2020

Choose a reason for hiding this comment

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

@joshgoebel I'm confused, this language improvement is not part of the 10.4.0-beta1 right?

Copy link
Member

@joshgoebel joshgoebel Nov 15, 2020

Choose a reason for hiding this comment

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

Correct really they heading should probably change. Betas aren’t the same as releases though they’re rolling so things just keep getting added until we have a release

Copy link
Member

@joshgoebel joshgoebel Nov 15, 2020

Choose a reason for hiding this comment

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

So really the header and should be 10.4 rolling or some such. I’ll give it some thought. I only updated it because we have to update the header for production releases but that’s not true for beta releases so we have more flexibility there.

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.

2 participants