refactor(error): remove unnecessary error return

[BlackHole1]

If a function does not produce any errors, it should not return an error (except for functions defined by an interface). This will reduce the mental burden on users.

The AddDevices method was also added, on the side.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cfergeau for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

Hi @BlackHole1. Thanks for your PR.

I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[gbraad]

Thank you. I'll let @cfergeau decide as there might have been reasons, though he is currently on leave.

[gbraad]

The AddDevices method was also added, on the side. (If you are concerned about this, I will move it to another PR.)

Knowing Christophe, this should have been at least a separate commit, but best to make a PR specific to this with clear description.

[gbraad]

/ok-to-test

[BlackHole1]

this should have been at least a separate commit

Done

[cfergeau]

Sorry for the late reviews, you filed these PRs just before I went away for a while :)
I'll need to think some more about the error removal from the *New changes, there are some plus and minuses to having it/not having it :)

[cfergeau]

This is the main reason why most *New() methods return an error, some of them require an error return, so for me it's better if they are consistent and all return an error, instead of having to remember it. This also makes the API future-proof if one day we need to return an error.

[BlackHole1]

Hey @cfergeau. I agree with what you said: maintaining a consistent style is also a way to reduce cognitive load.

In this current PR, I noticed that there are only two FooNew() methods that return an error, and I’m wondering if we could also make them not return an error to maintain a consistent style.

1. VirtioInputNew: This method calls dev.validate(), but it actually doesn’t need to because other FooNew() methods will call dev.validate() during the ToCmdLine and FromOptions stages. So it’s pretty simple to make this method no-err.
2. VirtioNetNew: This method needs to call net.ParseMAC to determine whether the input mac address is correct. If we change the function signature to: VirtioNetNew(macAddress net.HardwareAddr), could we avoid having this function return an error?

---

as you said: "This also makes the API future-proof if one day we need to return an error". is also a reason. But for me: The optimal design for now will surely not be optimal in the future, avoid over-design. When the future returns an error, we can consider ways to improve, such as changing the function name to: FooSetup. At this stage, FooNew only does one thing, which is to create a struct instance.

[cfergeau]

With the recent changes in main, this additional change is needed:

commit 6988cca7c0833a6a61ce4c77fad7686e1182951b (HEAD -> improve-error)
Author: Christophe Fergeau <[email protected]>
Date:   Wed Jan 24 17:37:02 2024 +0100

    fixup! refactor(error): remove unnecessary error return

diff --git a/pkg/config/virtio.go b/pkg/config/virtio.go
index 075f8fb..dc76245 100644
--- a/pkg/config/virtio.go
+++ b/pkg/config/virtio.go
@@ -470,10 +470,10 @@ func nvmExpressControllerNewEmpty() *NVMExpressController {
 // NVMExpressControllerNew creates a new NVMExpress controller to use in the
 // virtual machine. It will use the file at imagePath as the disk image. This
 // image must be in raw format.
-func NVMExpressControllerNew(imagePath string) (*NVMExpressController, error) {
+func NVMExpressControllerNew(imagePath string) *NVMExpressController {
        r := nvmExpressControllerNewEmpty()
        r.ImagePath = imagePath
-       return r, nil
+       return r
 }
 
 func virtioBlkNewEmpty() *VirtioBlk {
diff --git a/pkg/config/virtio_test.go b/pkg/config/virtio_test.go
index dfaccf7..804e160 100644
--- a/pkg/config/virtio_test.go
+++ b/pkg/config/virtio_test.go
@@ -43,7 +43,7 @@ var virtioDevTests = map[string]virtioDevTest{
                alternateCmdLine: []string{"--device", "virtio-blk,deviceId=test,path=/foo/bar"},
        },
        "NewNVMe": {
-               newDev: func() (VirtioDevice, error) { return NVMExpressControllerNew("/foo/bar") },
+               newDev: func() (VirtioDevice, error) { return NVMExpressControllerNew("/foo/bar"), nil },
                expectedDev: &NVMExpressController{
                        StorageConfig: StorageConfig{
                                DevName:   "nvme",

Can you also change the signed-off-by lines to use the same name as in PR #74?

[cfergeau]

Can you split these comment changes in a separate commit?

[cfergeau]

as you said: "This also makes the API future-proof if one day we need to return an error". is also a reason. But for me: The optimal design for now will surely not be optimal in the future, avoid over-design. When the future returns an error, we can consider ways to improve, such as changing the function name to: FooSetup. At this stage, FooNew only does one thing, which is to create a struct instance.

For internal/private API, I agree. For external API, changing the API in incompatible ways has to be balanced with the annoyances this will cause to users.
Removing the errors mean changing https://github.com/crc-org/crc/blob/2b661eb9eb2c57676e1f18b728e8aa5c025ba360/pkg/drivers/vfkit/driver_darwin.go#L187-L196 , changing https://github.com/containers/podman/blob/01b2243e73076a0e71a667153af1b98ecbf3b2f5/pkg/machine/applehv/vfkit.go#L10-L33

And I don't know if there are other users of vfkit's config code.
If we make the change now, and have to change it back in the future to add the error, then we'll have broken our API twice, and annoyed twice vfkit go users. This is why I'm very conservative with these changes. If we don't have a choice, I'll consider it, otherwise, I'll think hard about it before making the change.

(and API changes get even harder after the module has had a v1 release, as breaking API means we have to change the module path, which causes even more churn to go programs using vfkit)

[BlackHole1]

@cfergeau Okay, you convinced me. I will close this PR and create two new ones to address the following issues:

1. Add the AddDevices method
2. Improve comments

[cfergeau]

@cfergeau Okay, you convinced me. I will close this PR and create two new ones to address the following issues:

1. Add the `AddDevices` method

2. Improve comments

Sharing our different point of views made for an interesting discussion! Thanks for the 2 additional PRs :)