Skip to content

fix(bash): Add consistent highlighting for command-line options#4421

Open
WaterXiao-git wants to merge 1 commit into
highlightjs:mainfrom
WaterXiao-git:fix/bash-param-highlight
Open

fix(bash): Add consistent highlighting for command-line options#4421
WaterXiao-git wants to merge 1 commit into
highlightjs:mainfrom
WaterXiao-git:fix/bash-param-highlight

Conversation

@WaterXiao-git

Copy link
Copy Markdown

Add an OPTION mode to match short (-o) and long (--option=value) command-line flags, ensuring they are highlighted consistently across continuation lines instead of being partially matched by keyword or path patterns.

Closes #4288

Add an OPTION mode to match short (-o) and long (--option=value)
command-line flags, ensuring they are highlighted consistently
across continuation lines instead of being partially matched by
keyword or path patterns.

Closes highlightjs#4288
Comment thread src/languages/bash.js
// to ensure consistent highlighting across continuation lines.
const OPTION = {
className: 'attr',
begin: /(?:^|\s)--?\w[\w-]*(?:=?\S*)?\s*/,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not so sure about flags like

--mode=compact

Think perhaps the = should be operator (or nothing) and maybe compact should be a string? Thoughts?

Might be nice to see this a few different ways. CC @allejo Any thoughts?

@arturict arturict left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reproduced the current matcher against shell separators and redirections. There is a correctness issue beyond the open =value scoping question: (?:=?\S*)? is broader than an optional =value because = itself is optional.

For example, the mode consumes --foo;, --foo|grep, and --foo>out, which would hide shell operators/redirections inside hljs-attr; the surrounding whitespace is scoped too. Please scope only the option token and let the existing modes parse the remainder. A multi-part match could preserve the leading boundary without assigning it the attr scope.

This grammar change also needs markup cases for --mode=compact, --foo | grep, --foo>out, quoted/variable values, and short options. Since #4376, #4367, and #4369 address the same issue, it may be worth consolidating around one tested approach.

@joshgoebel

Copy link
Copy Markdown
Member

Agree, more works and tests needed here.

@arturict

Copy link
Copy Markdown
Contributor

If this option highlighting change touches multiple grammars, maybe include a compact list of tested languages to reduce regression risk.

@joshgoebel

Copy link
Copy Markdown
Member

Embedding is typically just running the grammar on a subset of code, so any tests added here would also apply to anywhere this grammar is embedded in another grammar... but it does seem we need a beefier chunk of tests here and then try to come up with the minimal regex that will cover our test cases.

And false positive cases. Keep piling on the false positive cases anyone.

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.

(Bash) Highlighted colors doesn't look correct

3 participants