Return concrete error type

[rakyll]

Go development practices favors returning concrete types over interfaces, so the users don't have to force cast the return value to the concrete type and the APIs self document the returned error type.

[tigrannajaryan]

Go development practices favors returning concrete types over interfaces, so the users don't have to force cast the return value to the concrete type and the APIs self document the returned error type.

@rakyll Can you please link to an authoritative source for this?

Effective Go does not seem to say anything about this. My first random pick from Go's builtin packages os.Open returns an error although the documentation says on error it will always *PathError.

[rakyll]

Sorry for responding late on this issue. Effective Go and CodeReviewComments don't mention -- they don't mention majority of the readability practices either but I opened a discussion on golang-dev to see if we can clarify it.

This case is different than os.Open. This is a constructor for a custom error type not any other function/method that returns a custom error.

Think about it. os.Open is not a constructor of errors. It performs a complicated sequence of syscalls to open a file descriptor. It behaves differently on every platform but returns a PathError regardless. When finalizing Go 1.0, it'd be a nightmare the freeze the APIs with PathError in it without giving any flexibility to return other types of errors in the future. According to the Hyrum's law, behavioral promises becomes hard promises as projects succeed. That's exactly what happened to the os APIs. Nowadays, people can easily tell if you break this behavior (golang/go#30491), so practiacally Go cannot break this soft promise anymore. But that's not our topic...

Our case is different. This is a constructor of a custom error type. Any function/method that wants to return an error of this type should choose wisely choose what to return and will probably return just an error.

[tigrannajaryan]

I am not against the practice and don't have a personal opinion about it. I don't recall seeing this pattern much, but that's just my anecdotal evidence, we can disregard it.

If we were to adopt the new approach I would like to see:

1. Some authoritative source explaining why it is the right thing to do.
2. No objections from Collector maintainers/approvers.
3. Our CONTRIBUTING guide to contain a reference to the authoritative source.

[jrcamp]

I think what @rakyll is saying makes sense. It's easier to go from more concrete to an interface than the other way around. ie, you can still do:

var err error = NewPartialScrapeError(...)

since the returned type adheres to error interface. Knowing the concrete type in certain cases is beneficial (as the change to the tests show).