Skip to content

feat(parser): handle inline comments in PEG command substitution#1006

Open
lu-zero wants to merge 1 commit intoreubeno:mainfrom
lu-zero:comments-in-command
Open

feat(parser): handle inline comments in PEG command substitution#1006
lu-zero wants to merge 1 commit intoreubeno:mainfrom
lu-zero:comments-in-command

Conversation

@lu-zero
Copy link
Copy Markdown
Contributor

@lu-zero lu-zero commented Feb 20, 2026

Exclude '#' from literal text when inside command context and add comment handling to command_piece() rule. This allows comments within
command substitutions to be properly parsed.

Exclude '#' from literal text when inside command context and add
comment handling to command_piece() rule. This allows comments within
 command substitutions to be properly parsed.
@github-actions
Copy link
Copy Markdown

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 17.07 μs 16.96 μs -0.11 μs ⚪ Unchanged
eval_arithmetic 0.15 μs 0.15 μs 0.00 μs ⚪ Unchanged
expand_one_string 1.52 μs 1.53 μs 0.01 μs ⚪ Unchanged
for_loop 22.30 μs 22.61 μs 0.31 μs ⚪ Unchanged
full_peg_complex 58.05 μs 58.37 μs 0.32 μs ⚪ Unchanged
full_peg_for_loop 6.13 μs 6.14 μs 0.01 μs ⚪ Unchanged
full_peg_nested_expansions 16.64 μs 16.87 μs 0.23 μs ⚪ Unchanged
full_peg_pipeline 4.24 μs 4.18 μs -0.05 μs ⚪ Unchanged
full_peg_simple 1.78 μs 1.76 μs -0.01 μs ⚪ Unchanged
function_call 2.10 μs 2.12 μs 0.02 μs ⚪ Unchanged
instantiate_shell 52.37 μs 52.74 μs 0.38 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 25198.47 μs 25512.83 μs 314.36 μs ⚪ Unchanged
parse_peg_bash_completion 2051.25 μs 2062.65 μs 11.40 μs 🟠 +0.56%
parse_peg_complex 19.70 μs 19.03 μs -0.67 μs 🟢 -3.39%
parse_peg_for_loop 2.00 μs 1.98 μs -0.02 μs ⚪ Unchanged
parse_peg_pipeline 2.07 μs 2.00 μs -0.06 μs 🟢 -3.14%
parse_peg_simple 1.09 μs 1.07 μs -0.02 μs ⚪ Unchanged
run_echo_builtin_command 15.27 μs 15.33 μs 0.06 μs ⚪ Unchanged
tokenize_sample_script 3.46 μs 3.42 μs -0.04 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-core/src/commands.rs 🟢 91.52% 🟢 91.74% 🟢 0.22%
brush-core/src/results.rs 🟢 80.16% 🟢 83.33% 🟢 3.17%
brush-interactive/src/basic/input_backend.rs 🟠 60% 🟠 62.86% 🟢 2.86%
brush-parser/src/word.rs 🟢 93.5% 🟢 93.52% 🟢 0.02%
Overall Coverage 🟢 73.41% 🟢 73.45% 🟢 0.04%

Minimum allowed coverage is 70%, this run produced 73.45%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1563 74.11
❗️ Error 18 0.85
❌ Fail 174 8.25
⏩ Skip 339 16.07
❎ Expected Fail 13 0.62
✔️ Unexpected Pass 2 0.09
📊 Total 2109 100.00

Copy link
Copy Markdown
Owner

@reubeno reubeno left a comment

Choose a reason for hiding this comment

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

I ran the 2 added YAML tests without the fix:

  • Comment in command substitution without newline fails - it does indeed fail, just with a different error than bash (so I'd expect the test to pass with ignore_stderr: true)
  • Comment in command substitution with newline - passes even without any code changes

Can you identify what case, in particular, you believe is a problem in the current code base?

@reubeno reubeno added the state: info needed An issue that requires additional info from the submitter label Feb 23, 2026
@lu-zero
Copy link
Copy Markdown
Contributor Author

lu-zero commented Feb 23, 2026

If it passes without the change, it is just me repeating the fix I had to make in winnow, but that's strange I was quite sure the original from Gentoo failed even with the stand alone tokenizer...

@lu-zero
Copy link
Copy Markdown
Contributor Author

lu-zero commented Feb 23, 2026

we can just keep the tests and consider later that bit if we want to emit comments and keep the peg parser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state: info needed An issue that requires additional info from the submitter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants