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

apps/nshlib: Save result and return ERROR if lib_get_tempbuffer() fails #2950

Conversation

JianyuWang0623
Copy link
Contributor

Summary

apps/nshlib: Save result and return ERROR if lib_get_tempbuffer() fails, missing goto ;-(

Impact

apps/nshlib: background

Testing

CI

@nuttxpr
Copy link

nuttxpr commented Jan 16, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements based on the provided information. While it provides a summary, it lacks crucial details.

Here's why:

  • Insufficient Summary Detail: While the summary mentions what was changed (error handling in lib_get_tempbuffer()), it doesn't explain why this change was necessary. Was there a bug? What problem did this solve? How does the fix work (what was the previous behavior and how is it different now)? Issue references are missing.

  • Impact Section is Too Brief: "apps/nshlib: background" is not descriptive enough. All the "Impact" sub-points need explicit "YES" or "NO" answers, along with descriptions for any "YES" responses. Even if the impact is minimal, it needs to be stated explicitly (e.g., "Impact on user: NO").

  • Testing is Inadequate: "CI" is not sufficient. The requirements ask for specific details about the local testing environment (host OS, compiler, target architecture, board, etc.) and, crucially, testing logs demonstrating the issue before the change and the corrected behavior after the change. Relying solely on CI testing isn't enough for a proper review.

This PR needs to be expanded to provide the missing information before it can be considered complete.

@xiaoxiang781216 xiaoxiang781216 merged commit 3108c27 into apache:master Jan 16, 2025
37 checks passed
@cederom
Copy link

cederom commented Jan 16, 2025

Good catch @JianyuWang0623 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants