diskfs / go-diskfs

MIT License
494 stars 112 forks source link

Use of errors.Unwrap makes it harder to use the API #237

Closed quite closed 1 month ago

quite commented 1 month ago

In https://github.com/diskfs/go-diskfs/pull/164 a couple of errors that the Create function can run into are unwrapped before being returned as part of an extended errors string. I'm sure that the intent was well-meaning. But I think that it makes things harder for me as a user of the API that the diskfs package provides.

I was about to use os.IsExist() on the error returned by Create, and couldn't understand why it would not be true...

I think that wrapping errors is the more sensible thing to do. Users of this API can then dig around as they wish for things to report to user of a program. But this is not the task of the API. Especially since initDisk is unexported so I can only use it through Create.

In the first instance, when OpenFile errors, I think Create should do something like:

return nil, fmt.Errorf("OpenFile failed: %w", err)

A caller of Create may on error do: return fmt.Errorf("Create failed: %w", err). And the printing of this would look like this (if the device was bootable.iso):

Create failed: OpenFile failed: open bootable.iso: file exists

This way the error does not drop any error info collected on the way. And API user can do as the wish. As it stands, I'll have to first check existence of the file myself before calling Create (well I'm not gonna do string compare on the error string :) Feels unnecessary really.

What do you think @shellwhale and @deitch

deitch commented 1 month ago

Yeah, this makes sense. I think the original motivation there was, "we are dropping errors, let us add them into the response". This just brings it one step further; if we properly wrap them, we get the information and the ability to work with it as an error. Open a PR?