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

feat: implement brace expansion #290

Merged
merged 1 commit into from
Dec 14, 2024
Merged

feat: implement brace expansion #290

merged 1 commit into from
Dec 14, 2024

Conversation

reubeno
Copy link
Owner

@reubeno reubeno commented Dec 13, 2024

This covers one of the last big remaining holes in expansion (the other is history expansion), albeit with complexity.

Resolves #42

@reubeno reubeno requested a review from Copilot December 13, 2024 18:03

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • brush-parser/src/error.rs: Evaluated as low risk
Comments suppressed due to low confidence (4)

brush-core/src/expansion.rs:474

  • The function brace_expand_if_needed lacks test coverage for cases where brace expansion parsing fails. Consider adding tests to ensure proper error handling and logging.
fn brace_expand_if_needed(&self, word: &'a str) -> Result<Vec<Cow<'a, str>>, error::Error> {

brush-core/src/expansion.rs:484

  • The parse_brace_expansions function call does not handle the case where the parse result is an error. This could lead to unexpected behavior. Consider handling the error case explicitly.
let parse_result = brush_parser::word::parse_brace_expansions(word, &self.parser_options);

brush-core/src/expansion.rs:498

  • The map operation on the result variable should handle empty strings properly. Consider adding a check to ensure that empty strings are handled as intended.
let result = result.map(|s| s.into()).collect();

brush-core/src/expansion.rs:1638

  • The generate_and_combine_brace_expansions function lacks test coverage for edge cases such as empty brace expressions and nested brace expressions. Consider adding tests for these cases.
.map(|piece| piece.generate().collect())
Copy link

github-actions bot commented Dec 13, 2024

Test Results

    2 files      9 suites   1m 22s ⏱️
  555 tests   555 ✅ 0 💤 0 ❌
1 096 runs  1 096 ✅ 0 💤 0 ❌

Results for commit 9f8e8e6.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 13, 2024

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
expand_one_string 3.49 μs 3.50 μs 0.00 μs ⚪ Unchanged
instantiate_shell 62.43 μs 62.24 μs -0.19 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 31964.62 μs 31718.78 μs -245.83 μs ⚪ Unchanged
parse_bash_completion 2901.66 μs 2807.54 μs -94.12 μs 🟢 -3.24%
parse_sample_script 4.31 μs 4.20 μs -0.11 μs 🟢 -2.60%
run_echo_builtin_command 93.75 μs 95.69 μs 1.94 μs 🟠 +2.07%
run_one_builtin_command 111.40 μs 113.59 μs 2.19 μs ⚪ Unchanged
run_one_external_command 2107.98 μs 1952.50 μs -155.49 μs 🟢 -7.38%
run_one_external_command_directly 1016.63 μs 1022.97 μs 6.35 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-core/src/expansion.rs 🟢 96.21% 🟢 96.1% 🔴 -0.11%
brush-core/src/shell.rs 🟢 77.86% 🟢 77.99% 🟢 0.13%
Overall Coverage 🟢 74.28% 🟢 74.56% 🟢 0.28%

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

@reubeno reubeno marked this pull request as ready for review December 13, 2024 23:35
@reubeno reubeno merged commit 839b803 into main Dec 14, 2024
17 checks passed
@reubeno reubeno deleted the braces branch December 14, 2024 04:43
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.

Implement brace expansion
1 participant