nuttxpr commented on PR #16167: URL: https://github.com/apache/nuttx/pull/16167#issuecomment-2789344918
[**\[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. Here's why: * **Insufficient Summary:** While it mentions the change (adding deprecated attribute support), it lacks crucial details. It doesn't explain *why* this change is necessary (what problem does it solve? What prompted this addition?). It vaguely mentions "Warning about usage of deprecated features," but doesn't specify which features are deprecated or why. How does defining `deprecated` with `__deprecated` address this? The break log is helpful context but doesn't replace a clear explanation of the change's mechanics. * **Impact Section Too Brief:** "N/A" is unacceptable. Even if the *user-facing* impact is minimal, there are still impacts to consider. At a minimum, it impacts the build (new macro definition). Consider if this change affects compatibility with older compilers. This section requires more thought. * **Insufficient Testing Information:** "ci-check" is not enough. The requirements explicitly ask for: * Build Host details (OS, CPU, Compiler version) * Target details (architecture, board, configuration) * Logs *before* the change * Logs *after* the change Simply stating "ci-check" doesn't provide any of this crucial information. Reviewers need to see evidence that the change works as intended on specific platforms. What if the CI only tests a limited set of configurations? **Specific Recommendations for Improvement:** * **Expand the Summary:** Explain the motivation behind adding deprecated attribute support. What specific problem does it solve? Which features will be marked as deprecated? Provide a clearer explanation of how the `__deprecated` macro usage addresses the issue. * **Complete the Impact Section:** Address *all* impact points, even if the answer is "NO." For the "build" impact, explicitly state the addition of the macro. Analyze potential compatibility issues. Consider documentation impact (does this change require documentation updates?). * **Provide Detailed Testing Information:** List all build hosts and targets used for testing, including specific versions. Include actual log output before and after the change to demonstrate the fix. Don't rely solely on CI results. By addressing these points, the PR will better meet the NuttX requirements and increase its chances of being accepted. -- 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]
