Refactor source code structure about syntax #433
Conversation
The syntax.rs file is getting too large, so I'm splitting the fmt::Display implementations to a separate module.
The existing implementation of `MaybeLiteral::extend_if_literal` for `[T]` was wrong in that it returned `Err(result)` where `result` was modified. This is a violation of the contract of the method, which should return `result` unmodified in case of failure. To fix this, the method is redefined as `extend_literal`, which is declared to modify `result` regardless of success or failure. The new method is defined to take a mutable reference to an `Extend<char>` object, instead of an ownership of it, because the ownership is not necessary for the method to work.
The syntax.rs file contained a lot of code about type conversions and environment-free expansions. To keep the file focused on the syntax definition, this commit moves the conversion code into a separate module, conversions.rs.
WalkthroughThe changes in this pull request update the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Poem
Warning Rate limit exceeded@magicant has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 1
🧹 Outside diff range and nitpick comments (15)
yash-syntax/CHANGELOG.md (2)
18-23: Enhance changelog entry with migration detailsWhile the changes to
MaybeLiteraltrait are well documented, consider adding:
- A "Breaking Changes" section to highlight the removal of
extend_if_literal- Migration steps for users to update from
extend_if_literaltoextend_literal- Example usage of the new
NotLiteralstructExample addition:
- The `syntax::NotLiteral` struct is added to represent the case where the method is unsuccessful. + + ### Breaking Changes + + - Removed `extend_if_literal` method from `MaybeLiteral` trait + + ### Migration Guide + + Replace usage of `extend_if_literal`: + ```rust + // Old + let mut result = Vec::new(); + if value.extend_if_literal(result) { + // handle success + } + + // New + let mut result = Vec::new(); + match value.extend_literal(&mut result) { + Ok(()) => // handle success, + Err(NotLiteral) => // handle failure + } + ```
Line range hint
1-24: Consider enhancing changelog metadataThe changelog follows good practices but could be improved by:
- Adding a target release date for version 0.13.0
- Including rationale for major changes (e.g., why the new case item terminators were added)
- Adding a "Security" section if any of the changes address security concerns
Example addition at the top:
## [0.13.0] - Unreleased + Target release date: December 2024 + + This release focuses on improving syntax parsing capabilities and error handling. + The new case item terminators (;&, ;|, ;;&) were added to support advanced + flow control in case statements, similar to bash's behavior.yash-syntax/src/syntax.rs (3)
701-702: Enhance module documentation forconversionsThe documentation comments for the
conversionsmodule are brief. Consider elaborating on the purpose and functionality of theUnquoteandMaybeLiteraltraits, as well as the types of conversions provided. This additional detail will improve clarity and assist future maintainers.
704-705: Improve documentation forimpl_displaymoduleSimilarly, the comments for the
impl_displaymodule could be expanded to provide more insights into how thestd::fmt::Displaytrait is implemented for the syntax types. Detailing any notable formatting decisions or edge cases will enhance understandability.
703-703: Streamline module declarations for readabilityConsider removing the blank line between the documentation comment and the
mod conversions;statement to group related code together. This can slightly improve code organization and readability.yash-syntax/src/syntax/conversions.rs (7)
23-24: Grammatical Correction in DocumentationIn the documentation comment, "If there is some quotes to be removed" should be "If there are some quotes to be removed" to match the plural noun "quotes".
Apply this diff to fix the grammatical error:
-/// If there is some quotes to be removed, the result will be `Ok(true)`. If no +/// If there are some quotes to be removed, the result will be `Ok(true)`. If no
29-29: Typo in Documentation: 'trail' should be 'trait'The word "trail" should be "trait" in the documentation comment.
Apply this diff to fix the typo:
-/// This trail will be useful only in a limited number of use cases. In the +/// This trait will be useful only in a limited number of use cases. In the
318-321: Address the TODO: Useextend_onefor EfficiencyThere's a TODO comment suggesting the use of
Extend::extend_one. Since Rust 1.51,extend_oneis stabilized and can be used to efficiently extend collections with single items without creating an iterator.Would you like assistance in updating the code to use
extend_onefor better performance?Apply this diff to implement the change:
fn extend_literal<T: Extend<char>>(&self, result: &mut T) -> Result<(), NotLiteral> { if let Literal(c) = self { - // TODO Use Extend::extend_one - result.extend(std::iter::once(*c)); + result.extend_one(*c); Ok(()) } else { Err(NotLiteral) } }
184-188: Consider Adding Validation inParam::variableThe
Param::variablemethod assumes the inputidis a valid variable name but does not perform validation. Consider adding checks to ensure thatidconforms to variable naming conventions to prevent potential issues.
74-79: Improper Use of Doc Test in DocumentationIn the documentation example, the code within the code block is not a complete example and may cause confusion when running doc tests.
Apply this diff to correct the documentation:
/// ``` /// # use yash_syntax::syntax::MaybeLiteral; /// # use yash_syntax::syntax::Text; /// # use yash_syntax::syntax::TextUnit::Backslashed; /// let backslashed = Text(vec![Backslashed('a')]); -/// assert_eq!(backslashed.to_string_if_literal(), None); +/// assert!(backslashed.to_string_if_literal().is_none()); /// ```
832-839: Usevec!Macro for Clarity in TestIn the test case, using the
vec!macro enhances readability and conciseness.Apply this diff to improve the code:
assert_matches!(assign.value, Scalar(value) => { assert_eq!( - value.units, - [ - WordUnit::Tilde("".to_string()), - WordUnit::Unquoted(TextUnit::Literal(':')), - WordUnit::Tilde("b".to_string()), - ] + value.units, + vec![ + WordUnit::Tilde("".to_string()), + WordUnit::Unquoted(TextUnit::Literal(':')), + WordUnit::Tilde("b".to_string()), + ] ); });
527-882: Consider Splitting Large Test Module into Smaller UnitsThe
testsmodule is quite large, encompassing multiple test functions. For better maintainability and readability, consider splitting it into smaller modules or reorganizing the tests using#[cfg(test)] mod module_nameblocks.yash-syntax/src/syntax/impl_display.rs (3)
905-920: Consider handling empty pipelinesIn the
pipeline_displaytest, you construct pipelines with commands, but there's no test case for an empty pipeline. According to the shell syntax, pipelines should contain at least one command. Adding a test for empty pipelines can help ensure the parser handles this scenario gracefully.Consider adding the following test:
#[test] fn pipeline_display_empty() { let p = Pipeline { commands: vec![], negation: false, }; assert_eq!(p.to_string(), ""); }
224-231: Potential formatting inconsistency inElifThenIn the
fmt::Displayimplementation forElifThen, you alternate between using{:#}and{}based on the formatter's alternate flag. Ensure that this conditional formatting is consistent with how theIfcommand and other compound commands are displayed.Consider reviewing the use of alternate formatting to maintain consistency.
334-339: Simplify match arms inAndOrDisplay implementationThe
fmt::Displayimplementation forAndOrcan be simplified by directly returning the strings associated with each variant.Apply this diff to simplify the implementation:
impl fmt::Display for AndOr { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - AndOr::AndThen => write!(f, "&&"), - AndOr::OrElse => write!(f, "||"), - } + f.write_str(match self { + AndOr::AndThen => "&&", + AndOr::OrElse => "||", + }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
yash-syntax/CHANGELOG.md(1 hunks)yash-syntax/src/syntax.rs(1 hunks)yash-syntax/src/syntax/conversions.rs(1 hunks)yash-syntax/src/syntax/impl_display.rs(1 hunks)
🔇 Additional comments (5)
yash-syntax/CHANGELOG.md (1)
Line range hint 1-400: Verify version numbers and chronological order
Let's ensure version numbers follow semantic versioning and dates are in chronological order.
✅ Verification successful
Version numbers and dates are correctly formatted and chronologically ordered
The verification shows that all version numbers in the CHANGELOG.md follow the semantic versioning format (MAJOR.MINOR.PATCH) and all dates follow the ISO 8601 format (YYYY-MM-DD). The dates are in chronological order from newest to oldest, which is the standard format for changelogs. The "Date order issue" messages in the output are false positives because the script compares dates across different files, but each changelog file maintains its own chronological order correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version numbers and dates in changelog
# Check if version numbers follow semantic versioning
echo "Checking version numbers..."
rg '## \[\d+\.\d+\.\d+\]' | while read -r line; do
if ! echo "$line" | grep -P '## \[\d+\.\d+\.\d+\] - \d{4}-\d{2}-\d{2}|Unreleased$' > /dev/null; then
echo "Invalid version/date format: $line"
fi
done
# Check if dates are in chronological order
echo "Checking chronological order..."
rg '## \[\d+\.\d+\.\d+\] - (\d{4}-\d{2}-\d{2})' -o --replace '$1' | tac | while read -r date; do
if [[ -n $prev_date && $date > $prev_date ]]; then
echo "Date order issue: $date comes after $prev_date"
fi
prev_date=$date
done
Length of output: 25626
yash-syntax/src/syntax.rs (1)
707-707: Verify all necessary items are re-exported from conversions
Ensure that all public traits and structs intended for external use are re-exported from the conversions module. Double-check if any additional items should be included alongside MaybeLiteral, NotLiteral, NotSpecialParam, and Unquote.
To verify, you can list all public items in src/conversions.rs:
Review the output and confirm that all necessary public items are re-exported in syntax.rs.
✅ Verification successful
All necessary items are re-exported from conversions
The review comment asked to verify if all public items from the conversions module are re-exported. Based on the examination of yash-syntax/src/syntax/conversions.rs, there are exactly four public items:
UnquotetraitNotLiteralstructMaybeLiteraltraitNotSpecialParamstruct
All of these items are correctly re-exported in the line:
pub use conversions::{MaybeLiteral, NotLiteral, NotSpecialParam, Unquote};There are no other public items in the module that need to be re-exported.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List all public items in the `conversions` module
# Command: Use `ast-grep` to find all public items
ast-grep --pattern 'pub $_' src/conversions.rs
Length of output: 114
Script:
#!/bin/bash
# First, let's find the correct path to the conversions module
fd conversions.rs
# Then, let's examine its content
rg "^pub " --type rust
Length of output: 60215
Script:
#!/bin/bash
# Let's examine the content of the conversions module
rg -A 5 "^pub " yash-syntax/src/syntax/conversions.rs
Length of output: 1096
yash-syntax/src/syntax/conversions.rs (2)
216-221: Potential Issue with write_unquoted Implementation in Switch
In the Unquote implementation for Switch, the write_unquoted method writes self.condition and self.r#type, then calls self.word.write_unquoted(w). Ensure that this ordering correctly represents the desired syntax and that there's no missing separator or formatting.
394-396: Documentation Clarification in Assign::try_from
The documentation states that = must be an unquoted equal sign, but the code does not explicitly check for a quoted equal sign. Ensure that this behavior is intentional, or update the code to enforce this constraint.
yash-syntax/src/syntax/impl_display.rs (1)
77-80: Clarify handling of TrimLength::Longest case
In the fmt::Display implementation for Trim, when TrimLength::Longest is selected, you call self.side.fmt(f)? a second time to duplicate the trim character (# or %). This results in ## or %% in the output.
Ensure that this behavior aligns with the intended shell syntax for longest matching patterns.
You can verify this by adding test cases or consulting the shell parameter expansion specification.
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
|
@coderabbitai Your summary seems to include changes from earlier pull requests. Could you create a summary that focuses on the diff for this particular pull request? |
|
@magicant: Thank you for bringing this to my attention. I'll regenerate the summary to focus exclusively on the diff for this particular pull request. ✏️ Learnings added
✅ Actions performedSummary regeneration triggered. |
Summary by CodeRabbit
New Features
;&,;|, and;;&.NotLiteralstruct for error handling in literal processing.fmt::Displayimplementations for various shell constructs for better string representation.Bug Fixes
SyntaxError::EsacAsPatternvariant to improve compliance with POSIX standards.Refactor
UnquoteandMaybeLiteraltraits, reorganizing functionality into new modules:conversionsandimpl_display.Chores