cockroachdb / errors

Go error library with error portability over the network
Apache License 2.0
2.09k stars 67 forks source link

Doc: Clarify distinction between WithDetail() and WithHint() #114

Closed omarkohl closed 1 year ago

omarkohl commented 1 year ago

By reading the source code and the existing documentation (e.g. README.md or code docstrings) I understand that WithDetail() and WithHint() can be used in the same way.

README states for both:

when to use: need to embark a message string to output when the error is presented to a human.

The source code hintdetail/with_detail.go and hintdetail/with_hint.go is also essentially identical.

So what is the difference? Maybe this library is trying to be non-prescriptive and giving its users freedom to use these functions and types as it suits them, but it is giving two mechanisms for apparently solving the same problem. Giving some guidance how this was originally intended is probably valuable to make usage more uniform and thereby make understanding and contributing to other people's code easier.

If I check the CockroachDB source code for instances of these functions I get the impression that WithDetail() is meant more for internal use to augment errors with additional information for debugging whereas WithHint() is meant for giving end users hints about what is wrong with the data they provided. But I might be misunderstanding or cherry-picking examples.

e.g.

err := errors.WithDetail(errors.Newf(
    "attempting to discard the zone configuration of a multi-region entity"),
    "discarding a multi-region zone configuration may result in sub-optimal performance or behavior",
)
return errors.WithDetail(
    pgerror.WithConstraintName(
        pgerror.Newf(
            pgcode.UniqueViolation, "%s %q", errMsg, constraintName,
        ),
        constraintName,
    ),
    fmt.Sprintf(
        "Key (%s)=(%s) is duplicated.", strings.Join(colNames, ","), strings.Join(valuesStr, ","),
    ),
)
if err != nil {
    topLevelError(errors.WithDetail(
        errors.Wrapf(err, "parsing statement %d", i+1), s.SQL),
        http.StatusBadRequest)
    return
}
err = errors.WithHint(err, "Try changing the CWD of the cockroach process to a writable directory.")
if err != nil {
    return errors.WithHint(
        errors.Wrapf(err, "job %d could not be loaded", jobs[i]),
        "The job may not have succeeded.")
}

How to improve

What I suggest is making some distinction in the README "Available wrapper constructors" section and in the WithDetail() and WithHint() docstring.

knz commented 1 year ago

Your interpretation is right.

Moreover, in the flatten functions details get concatenated but hints get deduplicated.

knz commented 1 year ago

addressed in https://github.com/cockroachdb/errors/pull/116.