nuttxpr commented on PR #14923: URL: https://github.com/apache/nuttx/pull/14923#issuecomment-2496903267
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details. Here's a breakdown: * **Summary:** While it explains the *what*, it's missing a clear *why*. Why is redirecting stdout to syslog necessary? What problem does it solve? Is there a specific use case driving this change? The mention of avoiding confusion with syslog/printf sequencing hints at the reason, but it needs to be more explicit. It also lacks issue references if applicable. * **Impact:** Marking "N/A" is insufficient. The description explicitly mentions that shell redirection (`<` and `>`) will no longer work as expected. This is a significant impact on users and needs to be documented as such. There are also potential impacts on: * **Build:** New config options (`SYSLOG_STDOUT`/`SYSLOG_STDOUT_PREFIX`) imply build system changes. * **Documentation:** This new feature requires documentation. * **Compatibility:** The change in shell redirection behavior is a backward compatibility break. * **Testing:** The testing is minimal. It demonstrates a single, simple case. More robust testing is needed, including: * Tests with different log levels. * Tests with long output strings. * Tests demonstrating the broken redirection behavior (and ideally a workaround or explanation if one exists). * Testing on real hardware, not just the simulator, if feasible. This helps identify potential platform-specific issues. * "Testing logs before change" is missing entirely. While it might not be applicable in this *specific* test case, it's a required section. Even a simple "N/A" would be better than omitting it. **In short:** The PR needs more detail and more thorough testing to meet the NuttX requirements. It needs to clearly explain the motivation, fully document the impact (especially the broken redirection), and include more comprehensive testing. -- 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
