-
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
Alias built-in #328
Alias built-in #328
Conversation
This commit modifies the main function of the alias built-in to use the semantics implemented in the previous commit.
WalkthroughThe updates to the Changes
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 X ? TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (6)
- yash-builtin/src/alias.rs (4 hunks)
- yash-builtin/src/alias/semantics.rs (1 hunks)
- yash-builtin/src/lib.rs (1 hunks)
- yash-env/src/builtin.rs (1 hunks)
- yash/tests/scripted_test.rs (1 hunks)
- yash/tests/scripted_test/alias-p.sh (1 hunks)
Files skipped from review due to trivial changes (1)
- yash/tests/scripted_test.rs
Additional comments: 14
yash-builtin/src/alias.rs (4)
19-60: The documentation has been expanded to provide a comprehensive description of the
alias
built-in command, which aligns with the PR objectives. This should enhance the understanding of the command's functionality for both users and developers.70-76: The introduction of the
Command
struct for encapsulating parsed command line arguments is a good practice for code clarity and maintainability.81-96: The refactoring of the
main
function to be asynchronous is a significant change that should improve performance and allow for better integration with other asynchronous operations within theyash
shell.16-115: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [16-188]
The changes in
alias.rs
are consistent with the PR objectives and the AI-generated summary. The file now includes comprehensive documentation, an asynchronousmain
function, a newCommand
struct, and asemantics
module for execution logic.yash-builtin/src/alias/semantics.rs (2)
124-129: The logic in the
execute
method for defining an alias or printing its definition seems sound. Ensure that operands are properly sanitized and alias names are validated to avoid any potential issues.143-286: The tests provided are comprehensive and cover a variety of scenarios, including error cases. This aligns with the user's previous action of adding tests for error cases, ensuring robustness in the functionality of the alias built-in.
yash-builtin/src/lib.rs (1)
- 106-110: The change from
Box::pin(ready(alias::main(env, args)))
toBox::pin(alias::main(env, args))
indicates thatalias::main
is now an asynchronous function. Ensure that thealias::main
function is properly refactored to return aFuture
and that this change is consistent with the asynchronous behavior intended by the PR.yash-env/src/builtin.rs (1)
- 224-224: The addition of the
#[inline]
attribute to theDefault
andFrom<ExitStatus>
trait implementations forResult
is a good practice for small functions that are frequently called, as it can potentially improve performance by reducing function call overhead.Also applies to: 231-231
yash/tests/scripted_test/alias-p.sh (6)
34-35: The test case for removing a specific alias is marked as a TODO, indicating that the
unalias
built-in is required but not yet implemented. Ensure that the implementation ofunalias
is planned or tracked in the project's issues or roadmap to complete these test cases.137-142: This test case is checking the behavior of using an alias after assignment. It's important to ensure that the environment variables (
a
ands
) are correctly passed to the subshell and that the alias (s
) is correctly expanded tosh
. This test case helps confirm that aliases are properly resolved in the context of command execution.475-482: The test case
IO_NUMBER cannot be aliased
is ensuring that file descriptor numbers are not mistakenly treated as aliases. This is a good test to ensure that the shell correctly distinguishes between aliases and file descriptor numbers, which are a fundamental part of shell redirection syntax.523-530: The test case
aliases cannot substitute reserved words
is crucial for ensuring that aliases do not override shell keywords, which could lead to unexpected behavior. This test maintains the integrity of the shell's parsing logic by confirming that reserved words are not affected by alias substitution.597-605: The test case
recursive alias
is testing the behavior of an alias that refers to itself indirectly through another alias. This is an advanced scenario that can potentially lead to infinite loops or stack overflows if not handled correctly by the shell.607-616: The test case
alias in command substitution
is verifying that an alias defined within a function is used when a command substitution is performed within that function. This test ensures that the scope of alias definitions is respected within command substitutions.
Summary by CodeRabbit
New Features
alias
command with detailed descriptions and support for global aliases.Bug Fixes
Refactor
alias
command implemented.Command
struct to encapsulate command line arguments.semantics
module to handle alias execution logic.Tests
Documentation