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

fix(for): correct semantics for "for" without "in" #348

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Conversation

reubeno
Copy link
Owner

@reubeno reubeno commented Jan 21, 2025

When in is not specified with a for clause, we need to enumerate positional arguments instead (i.e., $@).

This scenario was getting parsed correctly, but not correctly interpreted.

Resolves #342. Thanks to @ko1nksm for identifying the problem and filing a bug report on it!

@reubeno reubeno requested a review from Copilot January 21, 2025 18:22
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

shell.trace_command(std::format!(
"for {} in {}",
self.variable_name,
unexpanded_values.iter().join(" ")
))?;
} else {
shell.trace_command(std::format!("for {}", self.variable_name,))?;
Copy link
Preview

Copilot AI Jan 21, 2025

Choose a reason for hiding this comment

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

The trace message for the case without 'in' is unclear. It should indicate that positional parameters are being used. Suggestion: shell.trace_command(std::format!("for {} (using positional parameters)", self.variable_name))?;

Suggested change
shell.trace_command(std::format!("for {}", self.variable_name,))?;
shell.trace_command(std::format!("for {} (using positional parameters)", self.variable_name))?;

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link

Test Results

    3 files     14 suites   6m 6s ⏱️
  614 tests   614 ✅ 0 💤 0 ❌
1 828 runs  1 828 ✅ 0 💤 0 ❌

Results for commit f08c055.

Copy link

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 16.50 μs 16.33 μs -0.18 μs ⚪ Unchanged
eval_arithmetic 0.18 μs 0.18 μs 0.00 μs ⚪ Unchanged
expand_one_string 1.58 μs 1.61 μs 0.03 μs ⚪ Unchanged
for_loop 32.57 μs 32.98 μs 0.41 μs ⚪ Unchanged
function_call 3.33 μs 3.34 μs 0.01 μs ⚪ Unchanged
instantiate_shell 48.62 μs 49.03 μs 0.40 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 22252.98 μs 22072.52 μs -180.46 μs ⚪ Unchanged
parse_bash_completion 1665.44 μs 1667.31 μs 1.87 μs ⚪ Unchanged
parse_sample_script 1.78 μs 1.77 μs -0.01 μs ⚪ Unchanged
run_echo_builtin_command 16.08 μs 15.86 μs -0.22 μs ⚪ Unchanged
run_one_external_command 2394.58 μs 2443.91 μs 49.33 μs 🟠 +2.06%
tokenize_sample_script 2.83 μs 2.94 μs 0.11 μs 🟠 +3.96%

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-core/src/builtins/set.rs 🟠 60.8% 🟠 61.93% 🟢 1.13%
brush-core/src/commands.rs 🟢 86.83% 🟢 86.9% 🟢 0.07%
brush-core/src/interp.rs 🟢 91.07% 🟢 91.18% 🟢 0.11%
brush-core/src/jobs.rs 🔴 43.1% 🔴 37.07% 🔴 -6.03%
brush-parser/src/parser.rs 🟢 99.05% 🟢 99.16% 🟢 0.11%
Overall Coverage 🟢 74.73% 🟢 74.68% 🔴 -0.05%

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

@reubeno reubeno merged commit 925f829 into main Jan 21, 2025
18 checks passed
@reubeno reubeno deleted the for-without-in branch January 21, 2025 21:40
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.

The for loop without in does not iterate correctly
1 participant