Allow esac as the first pattern of a case branch
#426
Conversation
POSIX has been changed to allow `esac` as the first pattern of a case branch. This commit implements this change in the parser.
WalkthroughThe pull request introduces several changes across the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
yash-cli/CHANGELOG-bin.md (1)
15-17: LGTM with a minor suggestion.The changelog entry accurately documents the POSIX.1-2024 compliance change and provides a clear example. Consider adding a reference to the specific POSIX.1-2024 section that defines this behavior, similar to how other entries in the changelog reference POSIX (e.g., the entry in version 0.1.0-beta.1).
Apply this diff to enhance the documentation:
- The shell's syntax now allows `esac` as the first pattern of a case branch as in `case esac in (esac|case) echo ok; esac`. Previously, it was a syntax - error, but POSIX.1-2024 allows it. + error, but POSIX.1-2024 <insert-section-reference> allows it.yash-cli/tests/scripted_test/case-p.sh (2)
336-341: LGTM! Consider adding edge cases.The test case correctly validates the new POSIX-compliant behavior of allowing
esacas the first pattern.Consider adding test cases for:
- Multiple patterns including
esac(e.g.,esac|foo)- Quoted
esacpattern (e.g.,"esac")- Pattern with
esacas a substring (e.g.,fooesac)🧰 Tools
🪛 Shellcheck
[warning] 337-337: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
[warning] 337-337: This word is constant. Did you forget the $ on a variable?
(SC2194)
[warning] 337-337: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
Line range hint
347-355: Consider cleaning up temporary files.The test case correctly validates redirection handling. However, the temporary file 'redir_out' should be cleaned up after the test to prevent test pollution.
Consider adding cleanup:
case $(echo foo >&2) in $(echo bar >&2)) echo baz >&2;; esac 2>redir_out cat redir_out +rm -f redir_out __IN__ foo bar baz __OUT__🧰 Tools
🪛 Shellcheck
[warning] 337-337: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
[warning] 337-337: This word is constant. Did you forget the $ on a variable?
(SC2194)
[warning] 337-337: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
[warning] 343-343: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
[warning] 343-343: This word is constant. Did you forget the $ on a variable?
(SC2194)
[warning] 343-343: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
yash-syntax/CHANGELOG.md (1)
12-15: Consider adding POSIX reference for better traceability.The changelog entry clearly explains the change and rationale. To enhance traceability, consider adding a reference to the specific POSIX.1-2024 section that documents this requirement.
yash-syntax/src/parser/error.rs (1)
223-223: Consider updating the error messages for consistency.While the error messages for the deprecated
EsacAsPatternvariant are technically incorrect now (sinceesaccan be the first pattern), this is acceptable as the variant is deprecated and will be removed in a future version.However, consider adding a comment explaining why these messages are kept despite being incorrect, to prevent confusion for future maintainers.
#[allow(deprecated)] - EsacAsPattern => "`esac` cannot be the first of a pattern list", + // Note: This message is kept for backward compatibility despite being incorrect + // as `esac` is now allowed as the first pattern (POSIX.1-2024) + EsacAsPattern => "`esac` cannot be the first of a pattern list",Also applies to: 295-295
yash-syntax/src/parser/case.rs (1)
65-65: Consider adding a comment about POSIX complianceWhile the implementation is correct, it would be helpful to add a comment explaining that
esacis allowed as a pattern to comply with POSIX.1-2024 standards.+ // POSIX.1-2024 allows 'esac' as a valid pattern in case branches Token(_) => next_token.word,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
yash-cli/CHANGELOG-bin.md(1 hunks)yash-cli/tests/scripted_test/case-p.sh(1 hunks)yash-syntax/CHANGELOG.md(1 hunks)yash-syntax/src/parser/case.rs(2 hunks)yash-syntax/src/parser/error.rs(3 hunks)
🧰 Additional context used
🪛 Shellcheck
yash-cli/tests/scripted_test/case-p.sh
[warning] 337-337: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
[warning] 337-337: This word is constant. Did you forget the $ on a variable?
(SC2194)
[warning] 337-337: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
🔇 Additional comments (6)
yash-cli/tests/scripted_test/case-p.sh (1)
Line range hint 342-346: LGTM! Good backward compatibility test.
The test case properly validates that esac continues to work as a non-first pattern, ensuring backward compatibility.
🧰 Tools
🪛 Shellcheck
[warning] 337-337: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
[warning] 337-337: This word is constant. Did you forget the $ on a variable?
(SC2194)
[warning] 337-337: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
[warning] 343-343: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
[warning] 343-343: This word is constant. Did you forget the $ on a variable?
(SC2194)
[warning] 343-343: Use semicolon or linefeed before 'esac' (or quote to make it literal).
(SC1010)
yash-syntax/CHANGELOG.md (2)
19-24: LGTM!
The deprecation notice is well-documented with a clear explanation of why the error variant is no longer needed.
12-24: Version and status are appropriate.
The version bump to 0.12.1 is appropriate for these backward-compatible changes, and marking it as "Unreleased" is correct for work in progress.
yash-syntax/src/parser/error.rs (1)
138-138: LGTM! Good use of deprecation.
The addition of the deprecated EsacAsPattern variant is a clean way to handle the transition to allowing esac as a pattern while maintaining backward compatibility.
yash-syntax/src/parser/case.rs (2)
65-65: Implementation correctly allows esac as a pattern
The changes align with POSIX.1-2024 requirements by allowing esac as a valid pattern in case branches. The implementation maintains proper error handling while accommodating this new valid pattern case.
Also applies to: 319-322
319-322: Test coverage is comprehensive
The test suite thoroughly validates the new behavior, including:
- Basic usage of
esacas a pattern - Integration with existing pattern matching features
- Edge cases and error conditions
POSIX has been changed to allow
esacas the first pattern of a casebranch. This commit implements this change in the parser.
Summary by CodeRabbit
Release Notes for Version 0.1.1
New Features
esacas the first pattern in a case branch, aligning with POSIX.1-2024 standards.bgbuilt-in to correctly reflect the process ID of background jobs.execbuilt-in to prevent shell exit on command not found in interactive mode.Bug Fixes
Tests
casecommand to ensure compliance with various scenarios and edge cases.