Skip to content

Conversation

@jerpelea
Copy link
Contributor

Summary

This PR fixes the typos in the Code Comments of chip.h, allocateheap.c and start.c. The typos were discovered here:

Impact

RELEASE

Testing

CI

This PR fixes the typos in the Code Comments of chip.h, allocateheap.c
and start.c. The typos were discovered here:
- apache#15921

Signed-off-by: Lup Yuen Lee <[email protected]>
@jerpelea jerpelea changed the title arch/arm, risc-v: Fix typos in Code Comments {bp-15922} arch/arm, risc-v: Fix typos in Code Comments Mar 25, 2025
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small labels Mar 25, 2025
@nuttxpr
Copy link

nuttxpr commented Mar 25, 2025

[Experimental Bot, please feedback here]

No. The provided PR information is insufficient. While it mentions what files changed and links a related PR, it lacks critical details required by the NuttX PR template.

Here's why it's insufficient and what's missing:

  • Summary: While it mentions fixing typos, it doesn't explain what functional part of the code these comments relate to. This helps reviewers understand the context and potential impact of even seemingly minor changes. The link to the other PR is helpful but shouldn't replace a concise summary within this PR itself.

  • Impact: Simply stating "RELEASE" is not enough. While the impact might be minimal for typo fixes, the template requires explicit "YES/NO" answers for each category, along with descriptions for any "YES" responses. Even if all answers are "NO," stating so explicitly is important.

  • Testing: "CI" is insufficient. The template requires:

    • Specifics about the build host (OS, CPU, compiler).
    • Specifics about the target (architecture, board, configuration).
    • Actual testing logs before and after the change, demonstrating the correction. Relying solely on CI isn't enough, especially for seemingly minor changes that CI might not explicitly catch or log in a visible way.

In short, the PR needs to be significantly more detailed to meet the NuttX requirements. While the changes themselves may be small, the required information is essential for proper review and integration.

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jerpelea and @lupyuen :-)

@lupyuen lupyuen merged commit 9790ce8 into apache:releases/12.9 Mar 26, 2025
39 checks passed
@jerpelea jerpelea deleted the bp-15922 branch June 23, 2025 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants