Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

a few comment fixes #14834

Merged
merged 4 commits into from
Nov 18, 2024
Merged

a few comment fixes #14834

merged 4 commits into from
Nov 18, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Nov 18, 2024

Summary

a few comment fixes.

Impact

no functional changes are expected.

Testing

build tested.

@github-actions github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Area: Drivers Drivers issues Area: File System File System issues Size: S The size of the change in this PR is small labels Nov 18, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 18, 2024

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

No. The PR does not adequately address the NuttX requirements. While concise, it lacks crucial detail. Here's why:

  • Insufficient Summary: "A few comment fixes" is too vague. Which comments? In what files? What was wrong with them? What was changed? Even for minor changes, clarity is essential.
  • Missing Impact Assessment: While claiming "no functional changes," the PR doesn't explicitly address all impact areas. Even for comment changes, it needs to state "NO" for each impact category (user, build, hardware, documentation, security, compatibility) or explain if any are unexpectedly affected.
  • Inadequate Testing: "Build tested" is insufficient. Which build host and target? What does "tested" mean? The requirement asks for logs demonstrating the before/after state, even if it's just a clean build. Providing "no functional changes" doesn't eliminate the need for showing build success.

While small changes may seem trivial, adhering to these requirements ensures consistency, traceability, and ease of review. A better approach would be to provide specific details, even if brief, for each section.

@xiaoxiang781216 xiaoxiang781216 merged commit aa8aa44 into apache:master Nov 18, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: xtensa Issues related to the Xtensa architecture Area: Drivers Drivers issues Area: File System File System issues 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.

3 participants