Enable locations to be generated for the caller in libraries

[spearson78]

If a library uses this fault package but the caller does not (yet) then the reported error line is within the library which is not so useful to the caller. It makes sense to me that libraries should report the location of the caller to make it easier for non fault code to get an accurate location.
I however think that In user code this should stay as the line where the wrap is generated to enable the user to identify the relevant return statement.
To handle these 2 situations I separated out location generation in my fork and ensure that only 1 location is created per fault.Wrap
I have some additional changes on my branch relating to nil handling that I can separate out if this issue is accepted but my nil handling suggestion not.

[Southclaws]

Yeah I think you're right here, I went back and forth quite a bit on stack trace implementation and eventually settled on something quite simple though it's unfortunately opinionated.

I did consider making stack traces a decorator instead of integrated into the actual Fault library itself. But I opted for integration because it's pretty much a feature you'd never want to turn off. (and if you do, Fault probably isn't the right choice anyway)

One solution was to store a full stack trace (a slice of frame pointers) within the error chain and then when flattening/extracting metadata, locate the useful frames. However, this solution ended up being a bit too over-engineered.

It's worth considering a better solution long-term though. Perhaps it's as simple as just capturing two layers above the caller instead of just one and presenting one based on some heuristic?

[matdurand]

I had the same issue when I tried to create an internal error package in one project where I would create all my errors using fault. All my stacktraces were pointing to the same internal package which was useless.

Just brainstorming, but could we introduce the Option pattern to fault.New

func New(message string, opts ...Option) error {

and we could pass an option to calculate an alternative location, something like what Zap is doing, but with a Fault flavor, to calculate a new location by skipping a number of call stack. If you're a library, you know you probably want the 2nd call stack instead of the first, so you would pass the option.

Zap code as a reference:

func AddCallerSkip(skip int) Option {
	return optionFunc(func(log *Logger) {
		log.callerSkip += skip
	})
}

The downside is that we would need to sacrifice the fault.Newf function as it's already using vararg

[josephbuchma]

Hi! Thanks for the library and research around the topic of errors. It's fairly common for a project to have own "errors" package, and I think it is the right thing to do in many cases for following reasons:

• Tailoring API for specific project needs; some projects may benefit from customised error formatting etc...
• Avoid 3rd party dependency for something as core as error handling (remember when github.com/pkg/errors was archived?)

For that reason I think it would be great to have ability to create "Fault" instance which mirrors API of current fault package, but also allows to customise things like callerSkip or error formatting.

Something like this:

package myerrors

import "github.com/Southclaws/fault"

var fault = fault.NewInstance().WithMsgFormatter(myErrMsgFmtFunc).WithCallerSkip(4)

func New(msg string) error {
   return fault.New(msg)
}

// And the rest of myerrors API

Currently, I just copied source code of fault into internal package in my project to be able to build our project-specific errors package on top of fault.

[Southclaws]

This makes sense, Fault is designed as an error library library - a toolkit for building our your error handling solution with your own wrappers to chain in .Wrap calls so I'd welcome any improvements to the design in that regard.

An instance with configuration seems like a logical balance between keeping the top level interface very simple and providing flexibility. Definitely one to tackle for v1 #40

[mculleyhl]

@matdurand ,

That would be great. Are you already working on this? I created a wrapper so that my APIs are consistent for what I need. The location is always in my wrapper, of course.

e.g.

`func New(errMsg string) error {
return fault.New(errMsg)
}

func NewF(errMsg string, va ...any) error {
return fault.Newf(errMsg, va...)
}

func NewWithContext(errMsg string, ctx context.Context, kv ...string) error {
return fctx.Wrap(fault.New(errMsg), ctx, kv...)
}

func NewWithValues(errMsg string, kv ...string) error {
return fctx.Wrap(fault.New(errMsg), context.Background(), kv...)
}

func Wrap(err error, errMsg string) error {
return fault.Wrap(err, fmsg.With(errMsg))
}`

...

[Southclaws]

Yeah this is being worked on, I'll get a PR open for feedback from users soon!

I also do a similar thing with sentinel errors so there's a lot of re-use I could probably cut back on.

I frequently find myself and team members writing something like:

var errNotFound = fault.Wrap(fault.New("not found"), fmsg.WithDesc("not found", "The specified resource was not found"), ftag.With(ftag.NotFound))

Essentially, building your own is encouraged but I do need to make it easier. The core idea with Fault is that you can do anything you want as long as you satisfy the func (error) error interface.

[josephbuchma]

Adding location to sentinel errors at the point of definition is unnecessary. Would be nice to have something like func NewSentinel(msg string, opts ...func(error)error)error that does not add location info.

[matdurand]

@mculleyhl no I'm not.