lupyuen commented on PR #13494: URL: https://github.com/apache/nuttx/pull/13494#issuecomment-2357300091
> I suppose one way to find out whether Gemini understands our project requirements could be to give it a PR that does not meet the requirements and see if it gives the correct answer. @hartmannathan Remember the PR mentioned by Tomek? https://gist.github.com/lupyuen/99c3602cab74d7380944f0735364c51c#does-this-pr-meet-the-nuttx-requirements ```text ## Summary: Fix warning of unused variable 'devname' ... ## Impact: examples/sotest ... ## Testing: ./tools/configure.sh -l sim:sotest ... ``` Google Gemini AI thinks that it's OK. Gemini doesn't seem to understand the context (or do we feed to fine-tune our rules?) https://gist.github.com/lupyuen/99c3602cab74d7380944f0735364c51c#response-from-google-gemini-ai > **Overall Assessment**: The PR appears to adhere to the NuttX requirements. > **Points to Consider**: (1) While the testing demonstrates successful execution, including details about the build host (OS, CPU, compiler) would strengthen the verification process. (2) If the changes touch any configuration options or Kconfig files, mentioning their impact (if any) would be beneficial. > **Recommendation**: The PR seems suitable for submission, considering the minor nature of the changes and the provided testing information. I ran Gemini on the 4 latest PRs... ### Incomplete PR: Positive Example Gemini works OK for this **Incomplete PR**: https://gist.github.com/lupyuen/9b26677cb1d380a8fb66ea7f5c999fab#does-this-pr-meet-the-nuttx-requirements > **Overall Assessment:**: The PR seems **incomplete** in its current form. While it introduces a new feature for BCH devices (readonly configuration), several critical details are missing, making it difficult to assess its full impact and adherence to NuttX requirements. > **Recommendations:** To bring this PR in line with NuttX requirements, consider the following additions: > * **Summary:** Explain the mechanism of the readonly configuration. If applicable, reference related issues or PRs. > * **Impact:** Justify the claim of "NONE" impact. If there's truly no impact, explain why. > * **Testing:**: Provide comprehensive testing details as requested in the template. This includes the build host environment, target(s) used for testing, and testing logs showcasing the feature's behavior. ### Incomplete PR: Negative Example But Gemini missed out totally on this **Incomplete PR**: https://gist.github.com/lupyuen/70d6ff8515005663dd2059de31e23855#does-this-pr-meet-the-nuttx-requirements > Based on the provided information, this PR appears to be **compliant** with the NuttX requirements, but it could benefit from slightly more detail. > **Points for Improvement** > * **More detail in the summary**: While the summary is concise, it could be enhanced by briefly mentioning the specific macro being removed and why it was deemed useless. This additional context would make the purpose of the PR even clearer. > * **Testing logs**: Although the "Testing" section confirms that the changes were verified, it lacks specific logs. Even if the change is minor, it's good practice to include at least a brief "before" and "after" snippet to demonstrate that the build and basic functionality are unaffected. ### Partially-Complete PRs Gemini seems helpful for these 2 **Partially-Complete PRs**: https://gist.github.com/lupyuen/fee4f889b57dd6a744b8c5de0edbbdf8#does-this-pr-meet-the-nuttx-requirements > The PR seems to meet the NuttX requirements, primarily focusing on configuration simplification. However, the following points need attention: > * **Build Host details:** Specify the OS and compiler used for testing. > * **Documentation:** If any user-facing documentation exists around `devminor` configuration, it might require an update. > * **Clarity on the change:** While the linked PR might provide details, a concise explanation in this PR's description about the exact change and its benefits would be helpful for reviewers. https://gist.github.com/lupyuen/77d03e406352424fd7c4235185c5751d#does-this-pr-meet-the-nuttx-requirements > **Recommendations:** > 1. **Expand the summary:** Provide more details about the specific code changes and how `msync` is implemented. > 2. **Review impact:** Carefully consider if there are any potential impacts on other areas, even if they seem minor. > 3. **Provide complete testing information:** Include build host details, target information, and actual testing logs showing the behavior before and after the change. -- 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]
