nuttxpr commented on PR #14901: URL: https://github.com/apache/nuttx/pull/14901#issuecomment-2492984193
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR does **not** fully meet the NuttX requirements. While it addresses most sections, it has critical omissions and weaknesses: * **Summary:** Lacks clarity on *why* this workaround is necessary beyond the pending upstream merges. What specific problem does this solve *now*? "Relatively cumbersome" is vague. Explain the drawbacks of this approach clearly. The "Changes" section just repeats the summary. Detail *how* the conversion from `filep` to path is done. This is crucial information missing from the description. * **Impact:** While claiming no impact on users, the summary mentions cumbersome conversion. This implies potential performance implications, which should be addressed. Even if minor, it's an impact. Justify the NO impact claims more thoroughly. Is there *no* change in code size or execution time? * **Testing:** The provided testing is extremely weak. Simply showing "hello" and some file attributes proves almost nothing. Demonstrate specific scenarios where the previous implementation failed or was deficient and how this change addresses those. Provide more comprehensive logs, including the actual commands used and their full output. Testing only on `sim/nsh` is insufficient. Test on real hardware if possible. Crucially, show the "before change" logs to illustrate the problem this PR solves. Currently, there's no basis for comparison. **Specific Recommendations:** 1. **Elaborate on the "why":** Explain the concrete problem this PR solves *in the current NuttX version* due to the lack of `lfs_file_getattr/lfs_file_setattr`. What breaks or is deficient without this change? 2. **Detail the "how":** Explain the `filep` to path conversion mechanism precisely. Include code snippets if necessary. What are the performance implications of this conversion? 3. **Strengthen the "Impact" assessment:** Address *all* potential impacts, even if minor. Justify "NO" answers with concrete reasons. Don't dismiss potential performance impacts. 4. **Provide comprehensive testing:** * Show "before" logs demonstrating the problem. * Show "after" logs demonstrating the fix. * Test edge cases and error scenarios. * Test on more than just `sim/nsh`. Target real hardware if feasible. * Include the exact commands used and their complete output. By addressing these points, the PR will better meet NuttX requirements and be more likely to be accepted. -- 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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
