You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@nuttx.apache.org by "xiaoxiang781216 (via GitHub)" <gi...@apache.org> on 2024/11/02 18:47:34 UTC

[PR] fs: Fix mtd/sector512.c:554:19: error: incomplete definition of type 'struct partition_info_s' [nuttx]

xiaoxiang781216 opened a new pull request, #14609:
URL: https://github.com/apache/nuttx/pull/14609

   ## Summary
   
   by moving geometry and partition_info_s to include/fs/ioctl.h
   
   ## Impact
   
   Fix ci error reported here:
   https://github.com/apache/nuttx/actions/runs/11643419979/job/32424162668?pr=14605
   
   ## Testing
   
   ci
   
   


-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fs: Fix mtd/sector512.c:554:19: error: incomplete definition of type 'struct partition_info_s' [nuttx]

Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.

nuttxpr commented on PR #14609:
URL: https://github.com/apache/nuttx/pull/14609#issuecomment-2453095537

[**\[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 as described. While it provides some information, it lacks crucial details.

Here's what's missing:

* **Summary:** The summary is extremely vague. It needs to explain *why* moving those structures is necessary. What problem does this solve? Is it a bug fix? A refactoring? What was the original issue that led to this change? How does moving the structures address that issue?

* **Impact:** While it links to a CI error, this is insufficient. It needs to specifically address *all* the impact points:
* **Is new feature added? Is existing feature changed?** - This is likely a change to an existing feature (header file organization), so should be clarified.
* **Impact on user:** Even if the answer is NO, it should explicitly state NO.
* **Impact on build:** Even if the CI error is fixed, the description should explain *how* this change fixes the build. What was the build error and why did moving the structures resolve it?
* **Impact on hardware:** Explicitly state NO.
* **Impact on documentation:** Does this require documentation updates? If not, explicitly state NO.
* **Impact on security:** Explicitly state NO.
* **Impact on compatibility:** This is important! Does moving these structures break backward compatibility with any existing code? Justify why or why not.
* **Anything else to consider?** - Explicitly state N/A if nothing else needs consideration.

* **Testing:** Simply saying "ci" is not enough. While CI passing is good, the PR needs to show *specific* test results relevant to the change.
* **Build Host(s):** Specify the build host details used for testing.
* **Target(s):** Specify the target architecture and board configuration used for testing.
* **Testing logs before change:** Provide actual log output demonstrating the issue before the change. This could include the CI error message or other relevant logs.
* **Testing logs after change:** Provide log output showing the successful outcome after the change. This demonstrates that the fix works as intended.

In short, the PR needs to provide more context, explain the reasoning behind the change, and provide concrete evidence of testing and impact analysis. Just referencing a CI error is not sufficient for a proper review.

--
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fs: Fix mtd/sector512.c:554:19: error: incomplete definition of type 'struct partition_info_s' [nuttx]

Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.

xiaoxiang781216 merged PR #14609:
URL: https://github.com/apache/nuttx/pull/14609


-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org