Skip to content

Commit 17f402b

Browse files
committed
Reduce code duplication and fix README example
- Add Command::redirects() helper method to ast.rs - Simplify get_first_line/get_last_line using Command::line() - Simplify has_heredoc and collect_heredocs_impl using Command::redirects() - Fix README JSON example: remove incorrect 'line' on pipeline, add 'flags' field Net reduction of ~33 lines while improving maintainability.
1 parent 04f9065 commit 17f402b

File tree

3 files changed

+45
-79
lines changed

3 files changed

+45
-79
lines changed

README.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,6 @@ Output:
150150
```json
151151
{
152152
"type": "pipeline",
153-
"line": 1,
154153
"commands": [
155154
{
156155
"type": "for",
@@ -162,7 +161,7 @@ Output:
162161
"line": 2,
163162
"words": [
164163
{ "word": "echo" },
165-
{ "word": "$i" }
164+
{ "word": "$i", "flags": 1 }
166165
],
167166
"redirects": []
168167
}
@@ -180,6 +179,8 @@ Output:
180179
}
181180
```
182181

182+
Note: The `flags` field on words indicates special expansion handling (e.g., `flags: 1` means the word contains a variable expansion).
183+
183184
## Error Handling
184185

185186
Syntax errors are handled gracefully and return `ParseError::SyntaxError`:

src/ast.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,4 +328,22 @@ impl Command {
328328
| Self::Coproc { line, .. } => *line,
329329
}
330330
}
331+
332+
/// Get the redirects for this command, if it has any.
333+
/// Returns `None` for command types that don't support redirects directly.
334+
#[must_use]
335+
pub const fn redirects(&self) -> Option<&Vec<Redirect>> {
336+
match self {
337+
Self::Simple { redirects, .. }
338+
| Self::For { redirects, .. }
339+
| Self::While { redirects, .. }
340+
| Self::Until { redirects, .. }
341+
| Self::If { redirects, .. }
342+
| Self::Case { redirects, .. }
343+
| Self::Select { redirects, .. }
344+
| Self::Group { redirects, .. }
345+
| Self::Subshell { redirects, .. } => Some(redirects),
346+
_ => None,
347+
}
348+
}
331349
}

src/to_bash.rs

Lines changed: 24 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -349,79 +349,33 @@ fn write_pipeline(commands: &[Command], negated: bool, out: &mut String) {
349349
}
350350

351351
/// Get the line number of the first token in a command
352-
/// Get the first line number of a command (for determining where it starts)
352+
/// Get the first line number of a command (for determining where it starts).
353+
/// For List nodes, recurses into the left child to find the actual first command.
353354
fn get_first_line(cmd: &Command) -> Option<u32> {
354355
match cmd {
355-
Command::List { left, line, .. } => {
356-
// For lists, recurse into left to find the first command
357-
get_first_line(left).or(*line)
358-
}
359-
Command::Simple { line, .. }
360-
| Command::Pipeline { line, .. }
361-
| Command::For { line, .. }
362-
| Command::While { line, .. }
363-
| Command::Until { line, .. }
364-
| Command::If { line, .. }
365-
| Command::Case { line, .. }
366-
| Command::Select { line, .. }
367-
| Command::Group { line, .. }
368-
| Command::Subshell { line, .. }
369-
| Command::FunctionDef { line, .. }
370-
| Command::Arithmetic { line, .. }
371-
| Command::ArithmeticFor { line, .. }
372-
| Command::Conditional { line, .. }
373-
| Command::Coproc { line, .. } => *line,
356+
Command::List { left, .. } => get_first_line(left),
357+
_ => cmd.line(),
374358
}
375359
}
376360

377-
/// Get the last line number of a command (for determining where it ends)
361+
/// Get the last line number of a command (for determining where it ends).
362+
/// For List nodes, recurses into the right child to find the actual last command.
378363
fn get_last_line(cmd: &Command) -> Option<u32> {
379364
match cmd {
380-
Command::List { right, line, .. } => {
381-
// For lists, recurse into right to find the last command
382-
get_last_line(right).or(*line)
383-
}
384-
Command::Simple { line, .. }
385-
| Command::Pipeline { line, .. }
386-
| Command::For { line, .. }
387-
| Command::While { line, .. }
388-
| Command::Until { line, .. }
389-
| Command::If { line, .. }
390-
| Command::Case { line, .. }
391-
| Command::Select { line, .. }
392-
| Command::Group { line, .. }
393-
| Command::Subshell { line, .. }
394-
| Command::FunctionDef { line, .. }
395-
| Command::Arithmetic { line, .. }
396-
| Command::ArithmeticFor { line, .. }
397-
| Command::Conditional { line, .. }
398-
| Command::Coproc { line, .. } => *line,
365+
Command::List { right, .. } => get_last_line(right),
366+
_ => cmd.line(),
399367
}
400368
}
401369

402370
/// Check if a command has any heredoc redirects (requires newline after)
403371
fn has_heredoc(cmd: &Command) -> bool {
404-
let redirects = match cmd {
405-
Command::Simple { redirects, .. }
406-
| Command::For { redirects, .. }
407-
| Command::While { redirects, .. }
408-
| Command::Until { redirects, .. }
409-
| Command::If { redirects, .. }
410-
| Command::Case { redirects, .. }
411-
| Command::Select { redirects, .. }
412-
| Command::Group { redirects, .. }
413-
| Command::Subshell { redirects, .. } => redirects,
414-
Command::List { left, right, .. } => {
415-
return has_heredoc(left) || has_heredoc(right);
416-
}
417-
Command::Pipeline { commands, .. } => {
418-
return commands.iter().any(has_heredoc);
419-
}
420-
_ => return false,
421-
};
422-
redirects
423-
.iter()
424-
.any(|r| r.direction == RedirectType::HereDoc)
372+
match cmd {
373+
Command::List { left, right, .. } => has_heredoc(left) || has_heredoc(right),
374+
Command::Pipeline { commands, .. } => commands.iter().any(has_heredoc),
375+
_ => cmd
376+
.redirects()
377+
.is_some_and(|r| r.iter().any(|r| r.direction == RedirectType::HereDoc)),
378+
}
425379
}
426380

427381
/// Collect all heredocs from a command tree
@@ -433,21 +387,6 @@ fn collect_heredocs(cmd: &Command) -> Vec<&Redirect> {
433387

434388
fn collect_heredocs_impl<'a>(cmd: &'a Command, heredocs: &mut Vec<&'a Redirect>) {
435389
match cmd {
436-
Command::Simple { redirects, .. }
437-
| Command::For { redirects, .. }
438-
| Command::While { redirects, .. }
439-
| Command::Until { redirects, .. }
440-
| Command::If { redirects, .. }
441-
| Command::Case { redirects, .. }
442-
| Command::Select { redirects, .. }
443-
| Command::Group { redirects, .. }
444-
| Command::Subshell { redirects, .. } => {
445-
for r in redirects {
446-
if r.direction == RedirectType::HereDoc {
447-
heredocs.push(r);
448-
}
449-
}
450-
}
451390
Command::List { left, right, .. } => {
452391
collect_heredocs_impl(left, heredocs);
453392
collect_heredocs_impl(right, heredocs);
@@ -457,7 +396,15 @@ fn collect_heredocs_impl<'a>(cmd: &'a Command, heredocs: &mut Vec<&'a Redirect>)
457396
collect_heredocs_impl(c, heredocs);
458397
}
459398
}
460-
_ => {}
399+
_ => {
400+
if let Some(redirects) = cmd.redirects() {
401+
for r in redirects {
402+
if r.direction == RedirectType::HereDoc {
403+
heredocs.push(r);
404+
}
405+
}
406+
}
407+
}
461408
}
462409
}
463410

0 commit comments

Comments
 (0)