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

rpmsg/rpmsg_virtio_lite: rename rpmsg_virtio to rpmsg_virtio_lite #15336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luckyyaojin
Copy link

Summary

Because rpmsg_virtio_lite is a better name to inform user that the
rpmsg virtio (original name) is a lite implementation of rpmsg virtio.

Impact

None

Testing

CI

@github-actions github-actions bot added Arch: simulator Issues related to the SIMulator Area: Drivers Drivers issues Board: simulator Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Dec 25, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 25, 2024

[Experimental Bot, please feedback here]

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

Here's why:

  • Insufficient Summary: While the reason for the change is stated, the summary lacks detail. What functional part of the code changed? Was it just renaming files/variables? Were any code modifications made beyond renaming? How does this change work? Just stating it's a "better name" isn't enough.

  • Impact is likely understated: A name change, especially one involving a driver, can have broader impacts. Consider:

    • Documentation: If the name appears in documentation, it must be updated. The PR should state this explicitly and confirm the updates were made.
    • Compatibility: Does this change break existing user configurations or scripts? If existing users relied on the old name, even a simple rename can break their builds. This needs investigation and explanation. The impact shouldn't be dismissed as "None".
    • Build: Even if the build process itself doesn't change significantly, the generated output (e.g., module names, symbol names) might change, which could impact users.
  • Testing is inadequate: "CI" is not sufficient. The PR needs to demonstrate that the renaming didn't introduce regressions. Provide concrete examples of tests run before and after the change. Include relevant logs or output snippets. What platforms were tested? Simply relying on CI passing isn't enough, especially for a name change which might not be caught by automated tests that focus on functionality. Specific tests related to the renamed driver are necessary.

In short: The PR needs to be more thorough in explaining the technical details of the change, its potential impact, and the testing performed. Don't assume reviewers will understand the implications of what might seem like a small change. Provide all necessary information to demonstrate that the change is correct, well-considered, and safe.

Because rpmsg_virtio_lite is a better name to inform user that the
rpmsg virtio (original name) is a lite implementation of rpmsg virtio.

Signed-off-by: Bowen Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: simulator Issues related to the SIMulator Area: Drivers Drivers issues Board: simulator Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants