Implement ignore-eof shell option #413
Conversation
WalkthroughThe changes introduce enhancements to the interactive shell functionality, including the implementation of the Changes
Assessment against linked 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
The fields of OpenFileDescription are now pub(crate) instead of pub(super). This is because the fields are used in the tests of IgnoreEof, which is in a different module.
![coderabbitai[bot]](https://avatars.githubusercontent.com/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-cli/CHANGELOG-bin.md (1)
28-28: Consider rewording the sentences to avoid repetition.The static analysis tool has correctly flagged that three successive sentences begin with "Interactive shells". To improve readability, consider rewording the sentences or using synonyms to avoid the repetition.
Here's a suggestion:
-Interactive shells now ignore the `noexec` option. -Interactive shells now support the `ignoreeof` option. -Interactive shells now allow modifying the trap for signals that were ignored +The `noexec` option is now ignored by interactive shells. +Support for the `ignoreeof` option has been added to interactive shells. +Modifying the trap for signals that were ignored on the shell startup.Tools
LanguageTool
[style] ~28-~28: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...hells now ignore thenoexecoption. - Interactive shells now support theignoreeofopti...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- yash-cli/CHANGELOG-bin.md (1 hunks)
- yash-cli/CHANGELOG-lib.md (1 hunks)
- yash-cli/src/startup/input.rs (4 hunks)
- yash-env/CHANGELOG.md (2 hunks)
- yash-env/src/input.rs (1 hunks)
- yash-env/src/input/ignore_eof.rs (1 hunks)
- yash-env/src/system/virtual.rs (1 hunks)
- yash-env/src/system/virtual/file_system.rs (4 hunks)
- yash-env/src/system/virtual/io.rs (6 hunks)
- yash-semantics/src/runner.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- yash-semantics/src/runner.rs
Additional context used
Learnings (1)
yash-env/src/system/virtual.rs (1)
Learnt from: magicant PR: magicant/yash-rs#405 File: yash-cli/src/startup/input.rs:88-88 Timestamp: 2024-08-22T13:22:00.012Z Learning: The `isatty` function in the `yash-env` project now returns a `bool` instead of a `Result<bool>`.
LanguageTool
yash-cli/CHANGELOG-bin.md
[style] ~28-~28: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...hells now ignore thenoexecoption. - Interactive shells now support theignoreeofopti...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
yash-env/CHANGELOG.md
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...to the user. -input::IgnoreEof- ThisInputdecorator implements the behavi...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tem::virtual::FileBody::Terminal- This is a new variant ofFileBody` that rep...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Additional comments not posted (27)
yash-env/src/input.rs (1)
32-33: LGTM!The introduction of the
ignore_eofmodule and the export of theIgnoreEofentity align with the PR objectives and the goals outlined in the linked issue #408. The placement of the module in theyash-envcrate and the export of theIgnoreEofentity as a public API are consistent with the suggestions and requirements discussed in the issue.The code changes are approved.
yash-cli/CHANGELOG-bin.md (1)
28-28: LGTM!The changelog entry accurately reflects the new
ignoreeofoption feature for interactive shells, as described in the PR summary and linked issue.Tools
LanguageTool
[style] ~28-~28: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...hells now ignore thenoexecoption. - Interactive shells now support theignoreeofopti...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
yash-cli/CHANGELOG-lib.md (1)
34-43: Enhancements toprepare_inputfunction for interactive shell environments.The changes to the
startup::input::prepare_inputfunction significantly improve the user experience in interactive shell environments:
- The
yash_env::input::Reporterdecorator reports the job status.- The
yash_env::input::IgnoreEofdecorator ignores the EOF character for input from file descriptors, allowing the shell to continue processing after EOF. This aligns with the PR objective of implementing theignore-eofoption.- The
yash_prompt::Prompterdecorator displays the prompt before reading input, and its application has been expanded from only standard input to include input from any file descriptor.These enhancements collectively contribute to a more user-friendly interactive shell experience.
yash-cli/src/startup/input.rs (7)
32-32: LGTM!The code changes are approved.
36-36: LGTM!The code changes are approved.
69-82: LGTM!The code changes are approved.
Line range hint
95-104: LGTM!The code changes are approved.
126-126: LGTM!The code changes are approved.
133-140: LGTM!The code changes are approved.
146-170: LGTM!The code changes are approved.
yash-env/CHANGELOG.md (2)
12-19: LGTM!The additions are well-documented and provide a clear overview of the new functionality. The descriptions are concise and informative.
Tools
LanguageTool
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...to the user. -input::IgnoreEof- ThisInputdecorator implements the behavi...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tem::virtual::FileBody::Terminal- This is a new variant ofFileBody` that rep...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
23-25: LGTM!The changes are well-documented and provide a clear overview of the modifications. The descriptions are concise and informative.
yash-env/src/input/ignore_eof.rs (6)
27-45: Excellent module documentation!The module documentation provides a clear and comprehensive overview of the
IgnoreEofdecorator. It covers the purpose, conditions for effectiveness, and the behavior in handling EOF scenarios. Well done!
46-55: Well-structuredIgnoreEofstruct definition!The
IgnoreEofstruct is well-defined with appropriate fields and generic lifetime parameters. The use ofRefCellfor the environment is suitable for shared mutable access. Good job!
57-80: Well-implementednewfunction!The
newfunction is a well-designed constructor for theIgnoreEofstruct. It takes the necessary arguments and initializes the struct fields appropriately. The documentation comment provides clear explanations of each argument, making it easy to understand and use the constructor correctly. Great work!
82-109: Correctly implementednext_linefunction!The
next_linefunction properly implements the behavior of theIgnoreEofdecorator. It handles EOF scenarios by checking the relevant conditions and re-reading the input if necessary. The use of a loop with a maximum number of tries prevents infinite looping. The function also correctly accesses the environment usingRefCellandborrow(). Well done!
111-379: Comprehensive test module!The test module provides thorough test coverage for the
IgnoreEofdecorator. It covers various scenarios, including reading from the inner input, handling EOF, and considering different shell options and terminal status. The use ofVirtualSystemenables controlled testing environments. The tests effectively verify the expected behavior and error messages, ensuring the correctness of the decorator's implementation. Excellent work on the tests!
1-379: Review completed successfully!The
ignore_eof.rsfile has been thoroughly reviewed, and all code segments have been approved. The implementation of theIgnoreEofdecorator is well-structured, properly documented, and comprehensively tested. No further changes or improvements are necessary. Great job on this implementation!yash-env/src/system/virtual/file_system.rs (2)
Line range hint
215-254: LGTM!The addition of the
Terminalvariant to theFileBodyenum is a valid change to represent a terminal device in the file system. Thecontentfield allows storing virtual file content for the terminal device, and the comment provides clarity on its purpose and behavior.
285-285: LGTM!The
typeandsizemethods of theFileBodyenum are correctly updated to handle the newTerminalvariant. Thetypemethod maps it to theCharacterDevicefile type, and thesizemethod returns0, which is consistent with the comment indicating that it is a dummy device.Also applies to: 297-297
yash-env/src/system/virtual/io.rs (6)
41-56: LGTM!The changes to the
OpenFileDescriptionstruct are approved:
- Changing the visibility of the fields to
pub(crate)enhances their accessibility within the crate, potentially facilitating easier integration and manipulation of file descriptions.- The documentation comment changes contribute to a more uniform style.
93-95: LGTM!The change to include
FileBody::Terminalin the match arm of theis_ready_for_readingmethod is approved. This change indicates that terminal file types are now treated similarly to regular files and directories regarding readiness for reading operations, which may improve the overall behavior of file operations in the system.
108-110: LGTM!The change to include
FileBody::Terminalin the match arm of theis_ready_for_writingmethod is approved. This change indicates that terminal file types are now treated similarly to regular files and directories regarding readiness for writing operations, which may improve the overall behavior of file operations in the system.
126-126: LGTM!The change to include
FileBody::Terminalin the match arm of thereadmethod is approved. This change allows thereadmethod to handle terminal file types similarly to regular files, which may improve the overall behavior of file operations in the system.
172-172: LGTM!The change to include
FileBody::Terminalin the match arm of thewritemethod is approved. This change allows thewritemethod to handle terminal file types similarly to regular files, which may improve the overall behavior of file operations in the system.
220-220: LGTM!The change to return an error for
FileBody::Terminalin the match arm of theseekmethod is approved. This change prevents seeking operations on terminal file types, which is consistent with the behavior for symlinks and may improve the overall behavior of file operations in the system.yash-env/src/system/virtual.rs (1)
540-544: LGTM!The changes to the
isattyfunction look good. It now correctly checks if the file descriptor is associated with a terminal usingwith_open_file_description.
This makes sure the next prompt starts at the beginning of the line.
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/src/startup/input.rs (4 hunks)
Additional comments not posted (5)
yash-cli/src/startup/input.rs (5)
69-82: LGTM!The updated documentation comments accurately reflect the changes in the
prepare_inputfunction and provide clear guidance on how the decorators are applied based on the source and the interactive option.
Line range hint
95-104: LGTM!The changes improve the handling of the standard input source by setting it to non-blocking mode when required and using the new
prepare_fd_inputfunction to create the input object with the appropriate decorators.
126-126: LGTM!The change is consistent with the changes made for the
Stdincase and improves the code by using the newprepare_fd_inputfunction to create the input object with the appropriate decorators.
133-140: LGTM!The changes improve the handling of the command string source by creating a
Memoryinput object and applying theReporterdecorator if the interactive option is enabled. This is consistent with the behavior for other input sources.
147-170: LGTM!The new
prepare_fd_inputfunction encapsulates the logic for creating an input object from a file descriptor and applying the appropriate decorators based on the interactive option. This improves the code by centralizing the decorator logic and making it reusable for different input sources.
Closes #408
IgnoreEofdecoratorSummary by CodeRabbit
New Features
ignoreeofoption to prevent accidental shell exits on EOF.Bug Fixes
Documentation
ignore-eofoption and related functionalities.