nuttxpr commented on PR #2885: URL: https://github.com/apache/nuttx-apps/pull/2885#issuecomment-2531630536
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR does **not** fully meet the NuttX requirements. While it provides a good starting point, it lacks crucial information in several sections. Here's a breakdown of what's missing and how to improve it: * **Summary:** * **Missing:** A clear explanation of *why* this change is necessary. Simply stating it allows basic shell operations when the environment is disabled isn't enough. What problem does this solve? What use case does it enable? Why is it important to have these commands work without the environment? * **Missing:** What functional part of the code is changed? Be specific. Mention the files modified (e.g., `nshlib/builtin_cd.c`, etc.). * **Missing:** How *exactly* does the change work? Does it introduce a local CWD variable within the shell? How is it managed? What code paths are affected? * **Impact:** * **Incomplete:** Saying it "depends on" another PR isn't sufficient. You need to explain the impact *of this PR itself*, assuming the other PR is merged. Address *all* the points: * **Is new feature added?** If these commands didn't work at all before, then yes. * **Impact on user:** YES. Explain that users will now be able to use `cd`, `ls`, and `pwd` even with `CONFIG_DISABLE_ENVIRON` enabled. * **Impact on build:** Likely NO, but confirm. * **Impact on hardware:** Likely NO, but confirm. * **Impact on documentation:** YES. If the behavior changes, the documentation needs updating to reflect this new functionality. Mention whether you have included documentation updates in this PR. * **Impact on security:** Consider potential security implications. Does this change expand the attack surface in any way? Likely NO, but analyze and justify. * **Impact on compatibility:** Potentially YES. Does this change break any existing scripts or user expectations? Likely NO, but consider it. * **Testing:** * **Incomplete:** Provide more details about your build host: OS version, compiler version, etc. * **Incomplete:** Be specific about your target. "sim/nsh" isn't enough. Which simulator? What configuration? (e.g., `qemu-system-arm -M lm3s6965evb -kernel build/nuttx`). * **Insufficient testing:** While the logs show basic functionality, they are not comprehensive. Test edge cases and error conditions: * `cd` into non-existent directories. * `cd` into files. * `cd` with relative paths (`cd ../..`). * `ls` on empty directories. * `ls` with invalid paths. By addressing these points, you can significantly improve the quality of your PR and make it easier for reviewers to understand and approve your changes. A well-written PR increases the likelihood of it being merged quickly and smoothly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
