-
Notifications
You must be signed in to change notification settings - Fork 3
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toassign
match the new signature.The recent changes to the
assign
function include an additionalorig_name
parameter. However, the current function calls in the codebase do not reflect this updated signature.
- Update all calls to
assign
to include theorig_name
parameter.Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all function calls to
assign
match 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
CHANGELOG
files 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.2
with 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.0
with 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.0
is correctly added.yash-semantics/src/expansion/initial/param.rs (3)
88-90
: Approved: Improved error messaging.Including the parameter name in the
UnsetParameter
error improves the clarity of error messages.
325-326
: Approved: Updated test to reflect new error structure.The test now correctly checks for the
name
field in theUnsetVariable
error.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::apply
match the new signature.Verification successful
Verification successful.
The only instance of
switch::apply
in the codebase matches the new signature.
yash-semantics/src/expansion/initial/param.rs
Scripts 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
UnsetVariable
struct improves the clarity of error messages.
195-197
: Approved: Updated error conversion logic.The error conversion logic now correctly handles the
name
field in theUnsetVariable
error.
219-221
: Approved: Updated variable retrieval logic.The variable retrieval logic now correctly handles the
name
field in theUnsetVariable
error.
357-367
: Approved: Updated test to reflect new error structure.The test now correctly checks for the
name
field in theUnsetVariable
error.yash-semantics/src/expansion.rs (5)
134-135
: Approved: Improved error messaging.Including the parameter name in the
UnsetParameter
error improves the clarity of error messages.
156-156
: Approved: Updated error message logic.The error message logic now correctly handles the
name
field in theUnsetParameter
error.
171-172
: Approved: Updated error label logic.The error label logic now correctly handles the
name
field in theUnsetParameter
error.
Line range hint
179-191
:
Approved: Deprecatedrelated_location
method.The
related_location
method has been deprecated and replaced with theadditional_message
method, which provides more detailed error information.
197-231
: Approved: Added methods for detailed error information.The
additional_message
andfooter
methods 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
VacantError
struct and updating the error message format improves the clarity of error messages.
122-130
: Approved: Improved error context.Including the
vacancy
field in theNonassignableError
struct provides more context about the error.
134-137
: Approved: Improved error messaging.Including the parameter name in the
NotVariable
variant of theNonassignableErrorCause
enum 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_error
match the new signature.Verification successful
All function calls to
vacant_expansion_error
match 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.rs
Scripts 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
apply
match the new signature.Verification successful
Verify the function usage in the codebase.
The
apply
function insrc/expansion/initial/param/switch.rs
was updated to include anorig_name
parameter. Ensure that all calls to this function match the new signature.
Verified: Function usage matches the new signature.
All calls to the
apply
function insrc/expansion/initial/param/switch.rs
correctly match the new signature, including theorig_name
parameter.
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
9155d72
to
2afd67b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.0
is 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
NonassignableErrorCause
is 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
NonassignableError
has 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 theexpansion
module.The description of the changes in the
expansion
module is clear and concise. It accurately reflects the new additions to the error types and their methods.
27-31
: Detail the changes in theErrorCause
enum.The changes to the
ErrorCause
enum are clearly described, including the addition of thename
field and the enhancement of themessage
andlabel
methods.yash-semantics/src/assign.rs (2)
183-183
: Test thevacancy
field inperform_assignment_read_only
.The test correctly asserts that the
vacancy
field isNone
in theAssignReadOnlyError
.
70-70
: Ensure thevacancy
field is correctly set.The
vacancy
field is set toNone
in theAssignReadOnlyError
. Ensure this is the intended behavior and that the field is correctly used in other parts of the code.Verification successful
Ensure the
vacancy
field is correctly set.The
vacancy
field in theAssignReadOnlyError
struct is conditionally set toNone
orSome
based 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 thevacancy
field is correctly set.The
vacancy
field is set toNone
in 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 thevacancy
field is correctly handled.The
vacancy
field is handled in theadditional_annotations
method. Ensure this handling is correct and complete.Verification successful
Ensure the
vacancy
field is correctly handled.The
vacancy
field is handled in theadditional_annotations
method 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 thename
field is correctly set.The
name
field has been added to theUnsetParameter
variant. Ensure this field is correctly set and used in other parts of the code.Verification successful
Ensure the
name
field is correctly set.The
name
field in theUnsetParameter
variant is correctly set and used in the codebase.
yash-semantics/src/expansion.rs
yash-semantics/src/expansion/initial/arith.rs
yash-semantics/src/expansion/initial/param.rs
Scripts 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 thevacancy
field is correctly set.The
vacancy
field has been added to theAssignReadOnlyError
struct. Ensure this field is correctly set and used in other parts of the code.Verification successful
Ensure the
vacancy
field is correctly set.The
vacancy
field in theAssignReadOnlyError
struct is set to bothSome
andNone
in various instances. Ensure that the logic for setting this field is consistent and correct in all cases.
yash-semantics/src/expansion.rs
: Lines whereAssignReadOnlyError
is instantiated.yash-semantics/src/assign.rs
: Lines whereAssignReadOnlyError
is instantiated.yash-semantics/src/command/compound_command/for_loop.rs
: Lines whereAssignReadOnlyError
is instantiated.yash-semantics/src/expansion/initial/arith.rs
: Lines whereAssignReadOnlyError
is instantiated.yash-semantics/src/expansion/initial/param/switch.rs
: Lines whereAssignReadOnlyError
is instantiated.yash-builtin/src/typeset.rs
: Lines whereAssignReadOnlyError
is instantiated.yash-builtin/src/typeset/set_variables.rs
: Lines whereAssignReadOnlyError
is instantiated.yash-builtin/src/getopts/report.rs
: Lines whereAssignReadOnlyError
is 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 thefooter
method implementation.The
footer
method has been added to theErrorCause
enum. Ensure the method is correctly implemented and used in other parts of the code.Verification successful
Review the
footer
method implementation.The
footer
method is correctly implemented in theErrorCause
enum and is utilized within thefooters
method in theyash-semantics/src/expansion.rs
file. The method appears to be used appropriately, and there are no issues with its implementation or usage.
yash-semantics/src/expansion.rs
: Thefooter
method 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: Addedvacancy
field to error struct.The addition of the
vacancy
field to theAssignReadOnlyError
struct 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.
16cfa64
to
592a58f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-cli
shell binary updated to version0.1.0-beta.2
with improved prompt display and error messaging.yash-semantics
module for more informative messages.Documentation
yash-cli
documentation to clarify changes for binary and library crate.Bug Fixes
yash-cli
built-ins likebreak
,continue
,read
,source
, andset
.Chores
CHANGELOG
files.