8000 [EXTERNAL] docs(`0-shell` optionals): refactoring of 0-shell optional docs by heshamalmosawi · Pull Request #2998 · 01-edu/public · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[EXTERNAL] docs(0-shell optionals): refactoring of 0-shell optional docs #2998

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[EXTERNAL] docs(0-shell optionals): refactoring of 0-shell optional docs #2998

wants to merge 3 commits into from

Conversation

heshamalmosawi
Copy link
Contributor 8000

Why?

  1. Job Control Project
    This project has unclear and inconsistent requirements to the audit file, as well as having redundant audit files. This fix attempt to solve some of the inconsistencies, however in the solution overview will mention some of the unclear gaps still remaining in the requirements and left up to the maintainers to fix as they see fit.
  2. Scripting Project
    This project has inconsistent requirements, as well as having redundant audit files.

Implementation Details

  1. Scripting Project
  • This project has two audit files, one of them was removed and kept the audit/README.md file to stay consistent with other projects.
  • The project requirement as well as first question has been changed to Rust only to stay consistent with the main 0-shell project.
  1. Job Control Project
  • The biggest issue with this project was clarity in requirements, The requirements have been re-written to be clearer, requiring everything that is in the audit file. Two unclear points still remaining are the use of python, for this i have suggested allowing Command::new to spawn a python process. The other unclear point is the use of 2>1 >/dev/null, I was not sure about this point, as it was not required in the main project, so i left it up to the 01 staff to decide how to introduce this, as well as if my python fix is sufficient.
  • This project has two audit files, one of them was moved to audit/README.md and removed the one in the root to stay consistent with other projects.
  • The project requirement as well as first question has been changed to Rust only to stay consistent with the main 0-shell project.

@Oumaimafisaoui Oumaimafisaoui requested a review from pedrodesu July 11, 2025 08:36
@pedrodesu
Copy link
Contributor

Hello @heshamalmosawi! We appreciate your extensive PR. Your contributions are very much appreciated!

In general all the changes you presented seem good and well justified. In regards to the python situation - You make a great point. The point of the python command is to see that the process is spawned and then terminated, while the sleep runs.
Fortunately we can easily solve this with any command with the same usage as python - such as cat, which we tell them to implement. As such, I suggest you remove back the "allowing python" part on the job-control README, and changing the occurrences of python with cat inside of the job-control's audit file. Reinforcing to the student that cat should also have the expected behavior without arguments (echo back any input given) is also probably a good idea.
With these changes done everything should be sound and I can approve your contribution.

Once again, thank you so much for your time! We appreciate it.

@pedrodesu pedrodesu self-assigned this Jul 14, 2025
@pedrodesu pedrodesu added the 📕 Rust Rust label Jul 14, 2025
@heshamalmosawi
Copy link
Contributor Author
heshamalmosawi commented Jul 15, 2025 •
edited
Loading

Hello @pedrodesu,
Thanks for the feedback! I have updated the job-control project to use cat instead of python. To be consistent, I have added the cat no-argument mode into the main 0-shell project, to make sure that students have this option implemented, prior to going to the optional.

Although, there is still a gap with making the 2>1 & >/dev/null redirections a requirement, since I am not sure how to explain them, or what their scope is. My personal suggestion is for this use case, replacing the combination of both with a single custom alias (pre-defined name in the requirements) for this project that discards stdout, to maintain clarity & straight-forwardness in the project requirements. However your expertise and knowledge will probably more knowledgeable.

Please review the changes and feel free to edit, or have additions, or let me know so I can add those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrodesu pedrodesu Awaiting requested review from pedrodesu

At least 1 approving review is required to merge this pull request.

Assignees

@pedrodesu pedrodesu

Labels
📕 Rust Rust
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@heshamalmosawi @pedrodesu
0