nuttxpr commented on PR #14604: URL: https://github.com/apache/nuttx/pull/14604#issuecomment-2453002059
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description is a good start but could be improved to better meet the NuttX requirements. Here's a breakdown: **Strengths:** * **Clear Summary of Changes:** The summary clearly lists the specific changes made. * **Rationale:** The reasoning behind the changes (correct return values, readability) is explained. * **Testing Information:** Build host and target information are provided, along with logs before and after the change. **Weaknesses:** * **Impact Section Too Brief:** The impact section is very general. It needs to specifically address each point in the template (user impact, build impact, hardware impact, documentation, security, compatibility). Even if the answer is "NO", explicitly state it. * **Testing Logs Too Long and Lacking Specifics:** The logs are very long and make it difficult to see the actual impact of the changes. Trim them down to only the relevant parts and highlight the differences. Explain *what* you were testing and *how* the logs demonstrate the fix. * **Missing Issue References:** If this PR addresses a specific issue (or issues) in either the NuttX or NuttX Apps repositories, those should be linked in the Summary section. **Suggested Improvements:** **Summary:** * Add links to related issues if any exist. * Be more precise about *what* functional part of the code is changed (e.g., "The Mnemofs filesystem implementation in `fs/mnemofs`"). **Impact:** * **Impact on user:** NO. (While there are improvements, the user interface remains unchanged.) * **Impact on build:** NO. * **Impact on hardware:** NO. * **Impact on documentation:** YES. If the API return values changed, the documentation must be updated to reflect this. Indicate if the documentation has been updated as part of this PR or if a separate PR is needed. * **Impact on security:** NO (unless any of the fixed return codes had security implications, in which case explain them). * **Impact on compatibility:** Potentially YES if applications rely on the previous (incorrect) return codes. Explain this possibility and suggest mitigation strategies if any. * **Anything else to consider:** None. **Testing:** * Shorten the logs drastically. Focus on the specific lines that demonstrate the corrected return values and improved logging. For example, show the return value of `mnemofs_rmdir` when the path is not a directory, both before and after the fix. Similarly, show a snippet of the new, improved logging. * Add a description of the test case: "The `custom_hello` application was used to test file and directory operations. Before the change, incorrect return codes were observed in cases X, Y, and Z. After the change, the correct return codes were observed." **Example of Improved Testing Section:** ``` Testing: I confirm that changes are verified on local setup and works as intended: * Build Host(s): OS (Linux), CPU(Intel), compiler(GCC 13). * Target(s): arch(sim), sim:mnemofs. The `custom_hello` application was used to create and remove directories, demonstrating the fix for `mnemofs_rmdir`. Logs before change (incorrect return code): ``` nsh> mkdir /tmp/testdir nsh> rmdir /tmp/testdir/nonexistent nsh> echo $? 0 <-- Incorrect return code ``` Logs after change (correct return code): ``` nsh> mkdir /tmp/testdir nsh> rmdir /tmp/testdir/nonexistent nsh> echo $? -20 <-- Correct return code (ENOTDIR) ``` The improved logging is demonstrated by the following snippet from the `mnemofs_open` function: ``` mnemofs_open: [mnemofs | OPEN] Requested path is a.txt. mnemofs_open: [mnemofs | OPEN] Flags set as 0x7. ``` ``` By making these changes, your PR will be clearer, easier to review, and more likely to be accepted. Remember to be concise and focus on providing the necessary information in a readily digestible format. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
