Export built-in #319
Export built-in #319
Conversation
|
Your existing content is comprehensive and aligns with the provided instructions. Therefore, I will repeat it verbatim in my response: WalkthroughThe Changes
Poem
TipsChat with CodeRabbit Bot (
|
The export built-in should print variables with the name "export" instead of "typeset". This commit introduces a new struct PrintVariablesContext that contains the name to be used when printing variables.
magicant
force-pushed
the
export-builtin
branch
from
2850867 to
f77c07d
Compare
When the export built-in prints variables in the form of commands that invoke the export built-in, it should not include any options. This commit adds a new field to `PrintVariablesContext` to specify the options that may be printed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (9)
- yash-builtin/src/export.rs (1 hunks)
- yash-builtin/src/lib.rs (2 hunks)
- yash-builtin/src/typeset.rs (5 hunks)
- yash-builtin/src/typeset/print_variables.rs (24 hunks)
- yash-builtin/src/typeset/syntax.rs (1 hunks)
- yash/tests/scripted_test.rs (1 hunks)
- yash/tests/scripted_test/export-p.sh (1 hunks)
- yash/tests/scripted_test/simple-p.sh (1 hunks)
- yash/tests/scripted_test/typeset-y.sh (2 hunks)
Files skipped from review due to trivial changes (4)
- yash-builtin/src/typeset/syntax.rs
- yash/tests/scripted_test.rs
- yash/tests/scripted_test/simple-p.sh
- yash/tests/scripted_test/typeset-y.sh
Additional comments: 22
yash-builtin/src/lib.rs (2)
50-56: The addition of the
exportmodule is correctly placed within the conditional compilation block for theyash-semanticsfeature. This ensures that theexportcommand is only available when theyash-semanticsfeature is enabled, maintaining consistency with the handling of other built-in commands likeexec.123-127: The registration of the
exportcommand as a special built-in within theBUILTINSarray is done correctly. Ther#typeis set toSpecial, which is appropriate for built-in commands that affect the shell environment or state. Theexecutelambda correctly usesBox::pinto return aPin<Box<dyn Future<Output = ()>>>, which is consistent with the other built-in commands.yash-builtin/src/export.rs (6)
1-69: The module documentation is clear and provides a good overview of the
exportbuilt-in's functionality, options, operands, exit status, errors, and portability. It also mentions the dependency on thetypesetbuilt-in, which is helpful for understanding the implementation context.86-87: The
PORTABLE_OPTIONSarray currently only includes thePRINT_OPTION. If there are any other portable options that theexportbuilt-in should support according to POSIX or other standards, they should be included here. If this is the complete set of portable options, then this is fine as is.89-93: The
PRINT_VARIABLES_CONTEXTconstant is defined with an emptyoptions_allowedarray. If there are no options allowed for theexportbuilt-in when printing variables, this is correct. However, if there are options that should be allowed, they need to be included in this array.95-121: The
mainfunction is well-structured and handles parsing, interpreting, and executing the command. It also correctly handles errors and outputs results or failures. The use ofasyncis appropriate if the environment's I/O operations are asynchronous. However, ensure that all parts of the environment and thetypesetmodule are compatible with asynchronous execution to avoid potential issues with concurrency.109-110: The
unreachable!macro is used for theSetFunctionsandPrintFunctionsvariants of theCommandenum. This is appropriate if theexportbuilt-in should never deal with function-related commands. If there is a possibility that these cases could be reached, then proper error handling should be implemented instead.112-114: The
executemethod is called with thePRINT_VARIABLES_CONTEXTconstant. Ensure that this context is correctly configured for all scenarios whereexecutemight be called. If there are scenarios where a different context is required, the code should handle that accordingly.yash-builtin/src/typeset.rs (4)
340-360: The introduction of
PrintVariablesContextand the constantPRINT_VARIABLES_CONTEXTis a good design choice for managing the context in which variables are printed. It encapsulates the context information and allows for easy extension or modification in the future. However, ensure that theoptions_allowedfield inPRINT_VARIABLES_CONTEXTis correctly set to the array of options that are actually allowed for thetypesetbuilt-in. Ifself::syntax::ALL_OPTIONSincludes options that are not applicable totypeset, this could lead to incorrect behavior or output.443-458: The
executemethod's signature has been correctly updated to include theprint_variables_contextparameter, which is used to control the output format. This change is consistent with the pull request summary and is a good practice for extending functionality without modifying existing function signatures extensively. However, ensure that all calls toexecutethroughout the codebase are updated to pass this new parameter.597-607: The
to_messagefunction is a utility for converting a list ofExecuteErrorinstances into aMessagethat can be used for reporting failures. This is a good approach for error handling as it centralizes the conversion logic and makes it reusable. However, ensure that the function is actually being used wherever multipleExecuteErrorinstances need to be reported, and that the error reporting is consistent across the application.612-618: The
mainfunction has been updated to pass thePRINT_VARIABLES_CONTEXTto theexecutemethod, which aligns with the changes made to theexecutemethod's signature. This is a necessary update to ensure that thetypesetbuilt-in can utilize the new context for variable printing. Ensure that themainfunction is correctly handling the results and errors from theexecutemethod, and that the asynchronous nature of themainfunction is properly accounted for in the calling code.yash/tests/scripted_test/export-p.sh (1)
- 24-34: The TODO comment on line 24 suggests that the
evalbuilt-in is needed for this test case, but the test case is written as ifevalis already implemented. Ifevalis not yet implemented, this test case should be marked as incomplete or pending implementation.yash-builtin/src/typeset/print_variables.rs (9)
20-30: The
executemethod in thePrintVariablesstruct has been updated to include a new parametercontextof type&PrintVariablesContext. This change is part of the update to handle different contexts for variable printing. Ensure that all calls to this method have been updated to pass the new parameter.33-44: The
executemethod now sorts variables and prints them using theprint_onefunction, which has been updated to take the newcontextparameter. This is a logical change to support different printing contexts. Ensure that the sorting logic is consistent with the expected behavior and that theprint_onefunction is called with the correct context.90-133: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [57-108]
The
print_onefunction has been updated to include thecontextparameter and uses it to determine which options are allowed when printing variable attributes. TheAttributeOptionstruct is used to format the options based on theoptions_allowedfield from the context. Ensure that theoptions_allowedfield is correctly populated and used in all relevant calls toprint_one.
111-133: The
AttributeOptionstruct and itsDisplayimplementation have been modified to use theoptions_allowedfield. This change ensures that only the allowed options are printed. Verify that theoptions_allowedfield is correctly populated with the appropriate options and that theDisplayimplementation correctly reflects the variable attributes.193-200: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [151-212]
New tests have been added to verify the behavior of the
PrintVariablesstruct with the new context parameter. These tests check the output of theexecutemethod for different variable types and scopes. Ensure that the tests cover all relevant cases and that the expected output matches the actual behavior of theexecutemethod.
- 228-234: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [228-260]
The tests for printing variable names and values have been updated to reflect the changes in the
PrintVariablesstruct and theprint_onefunction. Ensure that the quoting of variable names and values is correct and that the tests accurately reflect the expected behavior.
- 278-284: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [278-511]
Additional tests have been added to verify the behavior of the
PrintVariablesstruct when printing variables with different attributes and scopes. These tests are important to ensure that the filtering of variables based on attributes and scope works as expected. Verify that the tests are comprehensive and that the expected output is correct.
- 534-640: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [520-547]
The test for selecting variables with multiple filtering attributes has been added to ensure that the
PrintVariablesstruct correctly handles cases where multiple attributes are used to filter variables. Verify that the test is correctly set up and that the expected output is accurate.
- 549-639: A new test module
non_default_contexthas been added to test the behavior of thePrintVariablesstruct with non-default contexts. These tests are crucial to ensure that the new context parameter is handled correctly and that the output is formatted as expected based on the context. Verify that the tests cover all relevant scenarios and that the expected output is correct.
magicant
force-pushed
the
export-builtin
branch
from
a50bc18 to
84e1bfc
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (9)
- yash-builtin/src/export.rs (1 hunks)
- yash-builtin/src/lib.rs (2 hunks)
- yash-builtin/src/typeset.rs (5 hunks)
- yash-builtin/src/typeset/print_variables.rs (24 hunks)
- yash-builtin/src/typeset/syntax.rs (1 hunks)
- yash/tests/scripted_test.rs (1 hunks)
- yash/tests/scripted_test/export-p.sh (1 hunks)
- yash/tests/scripted_test/simple-p.sh (1 hunks)
- yash/tests/scripted_test/typeset-y.sh (2 hunks)
Files skipped from review due to trivial changes (3)
- yash-builtin/src/typeset/syntax.rs
- yash/tests/scripted_test.rs
- yash/tests/scripted_test/simple-p.sh
Additional comments: 21
yash-builtin/src/lib.rs (2)
53-56: The addition of the
exportmodule is correctly placed within the conditional compilation block for theyash-semanticsfeature. This ensures that theexportcommand is only available when theyash-semanticsfeature is enabled, maintaining consistency with the handling of other built-in commands likeexec.123-127: The registration of the
exportcommand in theBUILTINSarray is correct. The command is marked asSpecial, which is appropriate for a shell built-in that affects the environment or has other special behaviors. The use ofBox::pinandreadyto wrap theexport::mainfunction is consistent with the other built-in commands, ensuring that the command can be executed asynchronously.yash-builtin/src/export.rs (5)
1-69: The documentation comments provide a clear and comprehensive overview of the
exportbuilt-in's functionality, options, operands, exit status, errors, portability, and implementation notes. This is good for maintainability and understanding of the code.86-87: The
PORTABLE_OPTIONSarray currently only includes thePRINT_OPTION. If there are other portable options that theexportbuilt-in should support according to POSIX or other standards, they should be included here. If this is the complete set of portable options, then this is fine as is.89-93: The
PRINT_VARIABLES_CONTEXTis defined with an emptyoptions_allowedarray. If there are no options allowed for printing variables in the context of theexportbuilt-in, this is correct. However, if there are options that should be allowed, they need to be included in this array.95-121: The
mainfunction is well-structured and handles parsing, interpretation, and execution of theexportcommand. It correctly sets the attributes and scope for variable and function commands and provides appropriate error handling. The use ofunreachable!macro for theSetFunctionsandPrintFunctionscommands ensures that these cases are not silently ignored if they somehow occur, which is good for robustness.95-121: The
mainfunction isasync, but there is no clear indication of any asynchronous operations within the function body itself. If there are no asynchronous calls being made, theasynckeyword could be removed to simplify the function signature. However, if theoutputandreport_*functions are indeed asynchronous, then this is correct.yash-builtin/src/typeset.rs (4)
340-360: The addition of the
PrintVariablesContextstruct and thePRINT_VARIABLES_CONTEXTconstant is a good implementation detail that allows for flexibility in the printing of variables. It encapsulates the context needed for printing, such as the built-in name and allowed options, which can be useful for future extensions or modifications to the printing behavior.443-458: The update to the
executemethod signature to include aprint_variables_contextparameter is a good change for maintainability. It allows the method to be more flexible and to support different contexts for printing variables. However, ensure that all calls toexecutethroughout the codebase are updated to pass this new parameter.597-607: The
to_messagefunction is a useful utility for converting a list of errors into a single message that can be reported. This is a good practice as it helps to consolidate error handling and reporting, making the code cleaner and more maintainable.612-618: The
mainfunction has been correctly updated to pass thePRINT_VARIABLES_CONTEXTto theexecutemethod. This change aligns with the updates made to theexecutemethod signature and ensures that the context is available when executing the command.yash-builtin/src/typeset/print_variables.rs (10)
20-30: The
executemethod has been updated to include a new parametercontextof type&PrintVariablesContext. This change will require all calls toPrintVariables::executeto be updated to pass this new parameter. Ensure that all such calls in the codebase have been updated accordingly.90-133: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [57-108]
The
print_onefunction has been updated to include a new parametercontext. This change is consistent with the addition of thecontextparameter in theexecutemethod and allows for more flexible printing behavior based on the provided context. The function now correctly uses thecontext.builtin_nameandcontext.options_allowedto format the output.
111-133: The
AttributeOptionstruct and itsDisplayimplementation have been updated to include and use theoptions_allowedfield. This change allows for more granular control over which options are displayed when printing variable attributes. Ensure that theoptions_allowedarray is correctly populated with the desired options when creating aPrintVariablesContext.193-200: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [151-212]
The tests have been updated to pass the new
contextparameter to theexecutemethod. The tests also verify the correct behavior of theexecutemethod with the updatedPrintVariablesstruct. The tests appear to be comprehensive and cover various scenarios, including printing single and multiple variables, arrays, and valueless variables.
- 228-234: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [228-260]
The tests for quoting variable names and values, as well as printing global and local variables, have been updated to reflect the changes in the
PrintVariablesstruct and itsexecutemethod. The tests ensure that the quoting and printing behavior is as expected.
- 278-284: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [278-293]
The tests for printing local variables only and handling the case where a variable is not found have been updated. The tests correctly expect an error when a variable is not found in the specified scope. This is a good practice to ensure that the error handling behavior of the
executemethod is correct.
- 315-321: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [315-352]
The tests for printing all global and local variables, as well as all local variables only, have been updated. The tests verify that the
executemethod correctly prints variables from the specified scope and that the output is sorted by name.
- 370-376: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [370-511]
The tests for printing attributes of valueless, scalar, and array variables, as well as selecting variables based on their attributes (readonly, exported, etc.), have been updated. These tests ensure that the
executemethod correctly handles variable attributes and filters variables based on the provided attributes.
- 534-640: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [520-547]
The tests for selecting variables with multiple filtering attributes and handling the case where a variable is not found have been updated. The tests verify that the
executemethod correctly filters variables based on multiple attributes and that it returns an error when a variable is not found.
- 549-639: The tests in the
non_default_contextmodule verify the behavior of theexecutemethod with different contexts. These tests ensure that theexecutemethod behaves correctly when differentbuiltin_nameandoptions_allowedvalues are provided in the context. This is a good practice to ensure that theexecutemethod is flexible and behaves as expected with different contexts.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- yash-builtin/src/typeset.rs (5 hunks)
- yash/tests/scripted_test/export-p.sh (1 hunks)
Additional comments: 7
yash-builtin/src/typeset.rs (4)
342-362: The introduction of
PrintVariablesContextand the constantPRINT_VARIABLES_CONTEXTis a good design choice for maintainability and clarity. It encapsulates the context for printing variables, which can be reused across different parts of the code. This aligns with the DRY principle and helps to avoid duplication.445-460: The
executemethod has been updated to accept a new parameterprint_variables_context. This change is significant as it affects the method signature and all calls to this method must now provide this context. It is important to ensure that all parts of the codebase that rely on this method are updated accordingly to avoid breaking changes.599-609: The
to_messagefunction is a good utility for converting a list of errors into a single message that can be reported. This is useful for error handling and reporting, as it provides a consistent way to display multiple errors to the user. However, it's important to ensure that the order of errors is preserved and that the context of each error is clear to the user.614-620: The
mainfunction now correctly passes thePRINT_VARIABLES_CONTEXTto theexecutemethod. This change is in line with the updates made to theexecutemethod's signature. The error handling usingreport_failureandto_messageis also appropriate, as it provides a structured way to handle and report errors.yash/tests/scripted_test/export-p.sh (3)
24-34: The test case 'reusing printed exported variables' is using the
evalbuilt-in to restore the exported variables. This is a valid test case, but it should be noted that usingevalcan be dangerous if not used carefully, as it can execute arbitrary code. It's important to ensure that the output ofexport -pis sanitized and safe to evaluate.36-46: The test case 'exporting with assignments' is testing the behavior of exporting a variable while assigning another one. The comment in lines 38-39 correctly states that POSIX requires
$ato persist after the export built-in, but it is unspecified whether$ais exported. This test case is valid and correctly tests the specified behavior.48-54: The test case 'read-only variable cannot be re-assigned' is testing the behavior of attempting to export a read-only variable. The expected behavior is that the export built-in fails and, since it is a special built-in, the non-interactive shell should exit. This test case is valid and correctly tests the specified behavior.
magicant
force-pushed
the
export-builtin
branch
from
fa70f87 to
f5dca5d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- yash-builtin/src/typeset.rs (5 hunks)
- yash/tests/scripted_test/export-p.sh (1 hunks)
Additional comments: 10
yash/tests/scripted_test/export-p.sh (5)
5-12: The test case 'exporting one variable' seems to be correct based on the previous discussion. The variable
ais exported before invoking the child shell, so the child shell should inherit the variablea. The expected output in line 11 is correct as2 bar.14-22: The test case 'exporting many variables' is also correct. The variables
a,b, andcare exported in the parent shell, which means they should be available in the child shell environment. The expected output in line 21 is correct as2 A B C.24-34: The test case 'reusing printed exported variables' is using the
export -pcommand to print the exported variables, unsettinga, and then re-evaluating the exported variables. This test case is correct as it verifies that the variableais restored to its exported value after being unset and re-evaluated.36-46: The test case 'exporting with assignments' is correct. It tests the behavior of exporting a variable with an assignment in the same command. The POSIX specification is correctly noted in the comments, and the test case verifies that
$apersists after the export built-in and that$bis exported.48-54: The test case 'read-only variable cannot be re-assigned' is correct. It tests that attempting to export a read-only variable should fail, and since
exportis a special built-in, the non-interactive shell should exit. The expected behavior is that theecho not reachedline should not be executed.yash-builtin/src/typeset.rs (5)
252-261: The documentation here is clear and provides a good overview of how the
typesetandexportbuilt-ins can share implementation details. This is a good example of code reuse and modularity. However, ensure that the shared code does not lead to tight coupling between the two built-ins, which could make future maintenance more difficult.346-366: The introduction of
PrintVariablesContextis a good design choice as it encapsulates the context needed for printing variables, making the code more maintainable and easier to understand. The use of a constantPRINT_VARIABLES_CONTEXTfor thetypesetbuilt-in is also a good practice as it avoids magic strings and repeated code.449-464: The
executemethod is well-structured and handles the different command types cleanly. However, ensure that the error handling is consistent and that all potential errors are being captured and reported correctly. It's also important to verify that thePrintVariablescommand is correctly using theprint_variables_contextto format the output as expected by thetypesetbuilt-in.603-613: The
to_messagefunction is a good utility for converting a list of errors into a single message that can be reported. This is useful for providing detailed feedback to the user when multiple errors occur during the execution of a command. Ensure that the message formatting is user-friendly and that all relevant information is included in the annotations.618-624: The
mainfunction is the entry point for thetypesetbuilt-in and it correctly parses the arguments, interprets them, and executes the resulting command. The use ofPRINT_VARIABLES_CONTEXTensures that the context for printing variables is consistent with thetypesetbuilt-in. It's important to ensure that the error reporting is clear and that the asynchronous nature of themainfunction does not introduce any race conditions or other concurrency issues.
export name=valuerather thantypeset -x name=valueSummary by CodeRabbit
New Features
exportbuilt-in command for the yash shell, enhancing variable management capabilities.Enhancements
typesetcommand with additional context handling for variable attributes and options.Testing
exportcommand to ensure POSIX compliance and correct behavior in various scenarios.exportcommand and removal of outdated TODOs.Documentation
exportcommand.