nuttxpr commented on PR #15541: URL: https://github.com/apache/nuttx/pull/15541#issuecomment-2589930825
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although some sections could be more detailed. Here's a breakdown with suggestions for improvement: * **Summary:** While the "what" is clear (replacing `CONFIG_NSH_LINELEN` with `CONFIG_LINE_MAX`), the *why* is missing. Explain the reason for this change. Is `CONFIG_NSH_LINELEN` deprecated? Does `CONFIG_LINE_MAX` offer advantages? Mentioning the related NuttX Apps PR is good. * **Impact:** Simply stating "boards/*" is insufficient. Be explicit about the impact. Does this change affect *all* boards? While it *likely* doesn't impact the user, build process, or hardware functionality directly, explicitly stating "NO" with brief justifications for each (e.g., "Build: NO - Only changes configuration, not build process itself") strengthens the PR. Documentation: YES if any documentation refers to `CONFIG_NSH_LINELEN`; otherwise NO. Security, Compatibility: Likely NO, but state it explicitly. * **Testing:** "Selftest with refresh.sh" is vague. What does this selftest entail? What were you checking? Provide more specific commands and expected outcomes. "CI" is also insufficient. Which CI checks were run and passed? Ideally, provide links to successful CI runs. While before/after logs aren't strictly necessary for a configuration change like this, providing snippets demonstrating the change in behavior (e.g., before: line limited to X characters; after: line limited to Y characters) would further enhance the PR. **Example of improved Impact and Testing sections:** **Impact:** * Is new feature added? NO * Is existing feature changed? YES (NSH line length configuration now uses a standardized setting) * Impact on user: NO (Users should not experience any functional changes unless they were relying on a non-default `CONFIG_NSH_LINELEN` setting. The new `CONFIG_LINE_MAX` provides a consistent experience.) * Impact on build: NO (Only configuration files are changed, not the build process itself) * Impact on hardware: NO (No hardware changes are involved) * Impact on documentation: YES (Documentation referencing `CONFIG_NSH_LINELEN` needs updating to `CONFIG_LINE_MAX`) * Impact on security: NO * Impact on compatibility: NO (Should be backward compatible as long as `CONFIG_LINE_MAX` provides sufficient line length) * Anything else to consider? None **Testing:** Build Host: Linux (Ubuntu 20.04), x86_64, GCC 9.4.0 Target: sim:qemu-x86_64 Tested the change by building and running NSH on the qemu-x86_64 target. Before the change, the maximum line length in NSH was limited by `CONFIG_NSH_LINELEN`. After the change, the maximum line length is now controlled by `CONFIG_LINE_MAX`. *Verification steps:* 1. Built NuttX for qemu-x86_64 with the original configuration (using `CONFIG_NSH_LINELEN`). 2. Ran NSH and observed the line length limit. 3. Applied the patch. 4. Rebuilt NuttX for qemu-x86_64 with the new configuration (using `CONFIG_LINE_MAX`). 5. Ran NSH and verified that the line length limit now corresponds to `CONFIG_LINE_MAX`. CI: All CI checks passed. [Link to CI run] By adding these clarifications, the PR becomes more robust and easier for reviewers to understand and approve. -- 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]
