Improve parameter expansion error message #385
Conversation
The `NonassignableError` enum has been replaced with a struct of the same name so that it can have a `Vacancy` field. This change is made to provide an additional annotation for the error message that describes the state of the parameter value that caused an attempt to the assignment. The ErrorCause::related_location method has been deprecated in favor of the new additional_message method. The new method can return a message without a location, indicating that the message should be annotated at the same location as the main message.
WalkthroughThe changes primarily involve improving error handling and message clarity across multiple modules of the Changes
Sequence Diagram(s)Possibly related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
The CHANGELOG.md file in the yash-cli crate is split into two files: CHANGELOG-lib.md for changes to the library crate and CHANGELOG-bin.md for changes to the shell binary as a whole. Since most shell users are unlikely to be interested in the library crate changes, we provide separate files to make it easier to find the relevant information.
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
yash-semantics/src/expansion/initial/param/switch.rs (1)
191-193: Ensure all function calls toassignmatch the new signature.The recent changes to the
assignfunction include an additionalorig_nameparameter. However, the current function calls in the codebase do not reflect this updated signature.
- Update all calls to
assignto include theorig_nameparameter.Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all function calls to
assignmatch the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `assign` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'assign'Length of output: 251172
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- check-release.sh (1 hunks)
- yash-cli/CHANGELOG-bin.md (1 hunks)
- yash-cli/CHANGELOG-lib.md (2 hunks)
- yash-cli/README.md (1 hunks)
- yash-semantics/CHANGELOG.md (2 hunks)
- yash-semantics/src/expansion.rs (6 hunks)
- yash-semantics/src/expansion/initial/arith.rs (4 hunks)
- yash-semantics/src/expansion/initial/param.rs (2 hunks)
- yash-semantics/src/expansion/initial/param/switch.rs (20 hunks)
Files skipped from review due to trivial changes (1)
- yash-cli/CHANGELOG-lib.md
Additional comments not posted (22)
yash-cli/README.md (1)
11-12: LGTM!The links to separate changelog files for the shell binary and the library crate are clear and correctly updated.
check-release.sh (1)
23-29: LGTM!The script now correctly loops through multiple
CHANGELOGfiles and checks for the release date, improving its robustness.yash-cli/CHANGELOG-bin.md (1)
1-42: LGTM!The changelog correctly documents the new version
0.1.0-beta.2with improvements and fixes, adhering to the format based on Keep a Changelog.yash-semantics/CHANGELOG.md (2)
8-37: LGTM!The changelog correctly documents the new version
0.4.0with added, changed, and deprecated features, adhering to the format based on Keep a Changelog.
120-120: LGTM!The link to the new version
0.4.0is correctly added.yash-semantics/src/expansion/initial/param.rs (3)
88-90: Approved: Improved error messaging.Including the parameter name in the
UnsetParametererror improves the clarity of error messages.
325-326: Approved: Updated test to reflect new error structure.The test now correctly checks for the
namefield in theUnsetVariableerror.Also applies to: 330-330
79-80: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
switch::applymatch the new signature.Verification successful
Verification successful.
The only instance of
switch::applyin the codebase matches the new signature.
yash-semantics/src/expansion/initial/param.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `switch::apply` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'switch::apply'Length of output: 529
Script:
#!/bin/bash # Description: Verify all function calls to `switch::apply` match the new signature across the entire codebase. # Test: Search for all occurrences of `switch::apply` in the codebase. rg --type rust 'switch::apply'Length of output: 169
yash-semantics/src/expansion/initial/arith.rs (4)
136-139: Approved: Improved error messaging.Including the variable name in the
UnsetVariablestruct improves the clarity of error messages.
195-197: Approved: Updated error conversion logic.The error conversion logic now correctly handles the
namefield in theUnsetVariableerror.
219-221: Approved: Updated variable retrieval logic.The variable retrieval logic now correctly handles the
namefield in theUnsetVariableerror.
357-367: Approved: Updated test to reflect new error structure.The test now correctly checks for the
namefield in theUnsetVariableerror.yash-semantics/src/expansion.rs (5)
134-135: Approved: Improved error messaging.Including the parameter name in the
UnsetParametererror improves the clarity of error messages.
156-156: Approved: Updated error message logic.The error message logic now correctly handles the
namefield in theUnsetParametererror.
171-172: Approved: Updated error label logic.The error label logic now correctly handles the
namefield in theUnsetParametererror.
Line range hint
179-191:
Approved: Deprecatedrelated_locationmethod.The
related_locationmethod has been deprecated and replaced with theadditional_messagemethod, which provides more detailed error information.
197-231: Approved: Added methods for detailed error information.The
additional_messageandfootermethods provide more detailed error information, improving the clarity and usefulness of error messages.Also applies to: 233-245
yash-semantics/src/expansion/initial/param/switch.rs (5)
97-101: Approved: Improved error messaging.Including the variable name in the
VacantErrorstruct and updating the error message format improves the clarity of error messages.
122-130: Approved: Improved error context.Including the
vacancyfield in theNonassignableErrorstruct provides more context about the error.
134-137: Approved: Improved error messaging.Including the parameter name in the
NotVariablevariant of theNonassignableErrorCauseenum improves the clarity of error messages.
Line range hint
246-259:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
vacant_expansion_errormatch the new signature.Verification successful
All function calls to
vacant_expansion_errormatch the new signature.The code changes are consistent and all instances of the function usage match the new signature.
yash-semantics/src/expansion/initial/param/switch.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `vacant_expansion_error` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'vacant_expansion_error'Length of output: 2086
Script:
#!/bin/bash # Description: Verify all function calls to `vacant_expansion_error` match the new signature across the entire codebase. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 'vacant_expansion_error'Length of output: 2086
Line range hint
271-300:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
applymatch the new signature.Verification successful
Verify the function usage in the codebase.
The
applyfunction insrc/expansion/initial/param/switch.rswas updated to include anorig_nameparameter. Ensure that all calls to this function match the new signature.
Verified: Function usage matches the new signature.
All calls to the
applyfunction insrc/expansion/initial/param/switch.rscorrectly match the new signature, including theorig_nameparameter.
src/expansion/initial/param/switch.rs: Verified all instances.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `apply` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'apply'Length of output: 49020
Script:
#!/bin/bash # Description: Verify all function calls to `apply` in `src/expansion/initial/param/switch.rs` match the new signature. # Test: Search for the function usage in the specific file. Expect: Only occurrences of the new signature. rg --type rust -A 5 'apply' yash-semantics/src/expansion/initial/param/switch.rsLength of output: 3758
magicant
force-pushed
the
param-names-in-error
branch
from
9155d72
to
2afd67b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- yash-cli/CHANGELOG-bin.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- yash-cli/CHANGELOG-bin.md
|
@coderabbitai summary |
Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
yash-semantics/CHANGELOG.md (3)
8-8: Clarify the release date.The version
0.4.0is marked as "Unreleased." Ensure to update this with the actual release date when the version is released.
20-23: Clarify the successor relationship.The entry mentions that
NonassignableErrorCauseis a successor toNonassignableError. It would be helpful to briefly explain why the change was made.- The `expansion::initial::NonassignableErrorCause` enum is a successor to - the previous `NonassignableError` enum. + The `expansion::initial::NonassignableErrorCause` enum replaces the + previous `NonassignableError` enum to provide more detailed error information.
32-33: Clarify the replacement ofNonassignableError.The entry mentions that
NonassignableErrorhas been replaced with a struct. It would be helpful to explain the benefits of this change.- The `expansion::initial::NonassignableError` enum has been replaced with a - struct of the same name so that it can have a `Vacancy` field. + The `expansion::initial::NonassignableError` enum has been replaced with a + struct of the same name to allow for the inclusion of a `Vacancy` field, + providing more detailed error information.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- yash-builtin/src/typeset.rs (1 hunks)
- yash-semantics/CHANGELOG.md (2 hunks)
- yash-semantics/src/assign.rs (2 hunks)
- yash-semantics/src/command/compound_command/for_loop.rs (1 hunks)
- yash-semantics/src/expansion.rs (10 hunks)
- yash-semantics/src/expansion/initial/arith.rs (5 hunks)
- yash-semantics/src/expansion/initial/param/switch.rs (22 hunks)
Files skipped from review as they are similar to previous changes (2)
- yash-semantics/src/expansion/initial/arith.rs
- yash-semantics/src/expansion/initial/param/switch.rs
Additional comments not posted (10)
yash-semantics/CHANGELOG.md (2)
12-18: Detail the changes in theexpansionmodule.The description of the changes in the
expansionmodule is clear and concise. It accurately reflects the new additions to the error types and their methods.
27-31: Detail the changes in theErrorCauseenum.The changes to the
ErrorCauseenum are clearly described, including the addition of thenamefield and the enhancement of themessageandlabelmethods.yash-semantics/src/assign.rs (2)
183-183: Test thevacancyfield inperform_assignment_read_only.The test correctly asserts that the
vacancyfield isNonein theAssignReadOnlyError.
70-70: Ensure thevacancyfield is correctly set.The
vacancyfield is set toNonein theAssignReadOnlyError. Ensure this is the intended behavior and that the field is correctly used in other parts of the code.Verification successful
Ensure the
vacancyfield is correctly set.The
vacancyfield in theAssignReadOnlyErrorstruct is conditionally set toNoneorSomebased on the context. The code handles this appropriately, ensuring the field is used correctly depending on the situation.
- yash-semantics/src/assign.rs
- yash-semantics/src/expansion.rs
- yash-semantics/src/expansion/initial/arith.rs
- yash-semantics/src/expansion/initial/param/switch.rs
- yash-semantics/src/command/compound_command/for_loop.rs
- yash-builtin/src/typeset.rs
- yash-builtin/src/getopts/report.rs
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `vacancy` field in the `AssignReadOnlyError`. # Test: Search for the `AssignReadOnlyError` struct usage. Expect: Correct usage of the `vacancy` field. rg --type rust -A 5 $'AssignReadOnlyError'Length of output: 13272
Script:
#!/bin/bash # Description: Verify the logic behind setting the `vacancy` field in `AssignReadOnlyError`. # Search for all instances where `AssignReadOnlyError` is instantiated and include surrounding lines for context. rg --type rust -A 10 -B 10 'AssignReadOnlyError {'Length of output: 158
Script:
#!/bin/bash # Description: Verify the logic behind setting the `vacancy` field in `AssignReadOnlyError`. # Search for all instances where `AssignReadOnlyError` is instantiated and include surrounding lines for context. rg --type rust -A 10 -B 10 'AssignReadOnlyError \{'Length of output: 23895
yash-semantics/src/command/compound_command/for_loop.rs (1)
97-97: Ensure thevacancyfield is correctly set.The
vacancyfield is set toNonein theAssignReadOnlyError. Ensure this is the intended behavior and that the field is correctly used in other parts of the code.yash-semantics/src/expansion.rs (4)
246-275: Ensure thevacancyfield is correctly handled.The
vacancyfield is handled in theadditional_annotationsmethod. Ensure this handling is correct and complete.Verification successful
Ensure the
vacancyfield is correctly handled.The
vacancyfield is handled in theadditional_annotationsmethod inyash-semantics/src/expansion.rs. Ensure this handling is correct and complete.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of the `vacancy` field in the `additional_annotations` method. # Test: Search for the `additional_annotations` method usage. Expect: Correct handling of the `vacancy` field. rg --type rust -A 5 $'additional_annotations'Length of output: 5792
142-143: Ensure thenamefield is correctly set.The
namefield has been added to theUnsetParametervariant. Ensure this field is correctly set and used in other parts of the code.Verification successful
Ensure the
namefield is correctly set.The
namefield in theUnsetParametervariant is correctly set and used in the codebase.
yash-semantics/src/expansion.rsyash-semantics/src/expansion/initial/arith.rsyash-semantics/src/expansion/initial/param.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `name` field in the `UnsetParameter` variant. # Test: Search for the `UnsetParameter` variant usage. Expect: Correct usage of the `name` field. rg --type rust -A 5 $'UnsetParameter'Length of output: 3842
113-123: Ensure thevacancyfield is correctly set.The
vacancyfield has been added to theAssignReadOnlyErrorstruct. Ensure this field is correctly set and used in other parts of the code.Verification successful
Ensure the
vacancyfield is correctly set.The
vacancyfield in theAssignReadOnlyErrorstruct is set to bothSomeandNonein various instances. Ensure that the logic for setting this field is consistent and correct in all cases.
yash-semantics/src/expansion.rs: Lines whereAssignReadOnlyErroris instantiated.yash-semantics/src/assign.rs: Lines whereAssignReadOnlyErroris instantiated.yash-semantics/src/command/compound_command/for_loop.rs: Lines whereAssignReadOnlyErroris instantiated.yash-semantics/src/expansion/initial/arith.rs: Lines whereAssignReadOnlyErroris instantiated.yash-semantics/src/expansion/initial/param/switch.rs: Lines whereAssignReadOnlyErroris instantiated.yash-builtin/src/typeset.rs: Lines whereAssignReadOnlyErroris instantiated.yash-builtin/src/typeset/set_variables.rs: Lines whereAssignReadOnlyErroris instantiated.yash-builtin/src/getopts/report.rs: Lines whereAssignReadOnlyErroris instantiated.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `vacancy` field in the `AssignReadOnlyError`. # Test: Search for the `AssignReadOnlyError` struct usage. Expect: Correct usage of the `vacancy` field. rg --type rust -A 5 $'AssignReadOnlyError'Length of output: 13272
204-217: Review thefootermethod implementation.The
footermethod has been added to theErrorCauseenum. Ensure the method is correctly implemented and used in other parts of the code.Verification successful
Review the
footermethod implementation.The
footermethod is correctly implemented in theErrorCauseenum and is utilized within thefootersmethod in theyash-semantics/src/expansion.rsfile. The method appears to be used appropriately, and there are no issues with its implementation or usage.
yash-semantics/src/expansion.rs: Thefootermethod is defined and used within this file.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `footer` method in the `ErrorCause` enum. # Test: Search for the `footer` method usage. Expect: Correct usage of the method. rg --type rust -A 5 $'footer'Length of output: 10886
yash-builtin/src/typeset.rs (1)
536-536: Enhancement: Addedvacancyfield to error struct.The addition of the
vacancyfield to theAssignReadOnlyErrorstruct enhances the detail in error messages. This change is beneficial for providing more context in error handling.
The vacancy is now reported not only for the `NonassignableError` but also for `AssignReadOnlyError`. This change is made to provide more information about the error. I chose to revert the addition of the `additional_message` method to the `ErrorCause` enum because it could not return more than one message. The annotations are now directly constructed in `impl MessageBase for Error` based on the error cause.
magicant
force-pushed
the
param-names-in-error
branch
from
16cfa64
to
592a58f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- yash-builtin/src/typeset.rs (1 hunks)
- yash-semantics/CHANGELOG.md (2 hunks)
- yash-semantics/src/assign.rs (2 hunks)
- yash-semantics/src/command/compound_command/for_loop.rs (1 hunks)
- yash-semantics/src/expansion.rs (10 hunks)
- yash-semantics/src/expansion/initial/arith.rs (5 hunks)
- yash-semantics/src/expansion/initial/param/switch.rs (22 hunks)
Files skipped from review as they are similar to previous changes (7)
- yash-builtin/src/typeset.rs
- yash-semantics/CHANGELOG.md
- yash-semantics/src/assign.rs
- yash-semantics/src/command/compound_command/for_loop.rs
- yash-semantics/src/expansion.rs
- yash-semantics/src/expansion/initial/arith.rs
- yash-semantics/src/expansion/initial/param/switch.rs
Summary by CodeRabbit
New Features
yash-clishell binary updated to version0.1.0-beta.2with improved prompt display and error messaging.yash-semanticsmodule for more informative messages.Documentation
yash-clidocumentation to clarify changes for binary and library crate.Bug Fixes
yash-clibuilt-ins likebreak,continue,read,source, andset.Chores
CHANGELOGfiles.