nuttxpr commented on PR #15551: URL: https://github.com/apache/nuttx/pull/15551#issuecomment-2591490809
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR appears to mostly meet the requirements, but is missing some key information. **What's good:** * **Summary provides a decent overview of the *what*.** It clearly lists the three changes being made. * **Testing mentions `ci-check`, implying some automated testing.** While not ideal, it's better than no testing information. **What's missing/needs improvement:** * **Summary lacks the *why*.** It states *what* is being moved/renamed, but not *why* these changes are necessary. What problem do they solve? What's the benefit? For example, why are `NAME_MAX`, etc. "unistd-specific and should not be filesystem dependent"? What issues did this dependency cause? Similarly, why rename `LIBC_OPEN_MAX`? Why clean up `CONFIG_NSH_LINELEN`? * **Impact is too dismissive.** "N/A" is almost never true. Even seemingly small changes can have impacts. Consider: * **Impact on user:** Will any applications relying on the old names/locations break? Even if the answer is no, *explain why*. * **Impact on build:** Are there any conditional compilation changes? Again, even if no, *explain why*. * **Impact on documentation:** This is explicitly mentioned as changed, so the impact *cannot* be N/A. Describe what documentation was changed. * **Impact on compatibility:** Moving definitions can introduce subtle compatibility issues. Justify why there are none. * **Testing is insufficient.** `ci-check` tells us nothing specific. What targets were tested? What tests were run? What were the *results* of those tests? Providing "before" and "after" logs, even if they are identical, demonstrates due diligence. If the logs are voluminous, consider linking to a CI run instead of pasting them directly. At a minimum, specify the host OS, compiler, target architecture, and board configuration used for testing. **In short:** The PR needs to expand on the *why* for each change and provide more concrete details regarding the impact and testing performed. Simply stating "N/A" or "ci-check" is not sufficient for a thorough review. -- 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
