celestiaorg / knuu

Integration Test Framework
Apache License 2.0
39 stars 34 forks source link

[Feature] Improved Errors Type #400

Closed MSevey closed 2 months ago

MSevey commented 4 months ago

Right now the way these errors are being defined and used is quite error prone, specifically as it relates to how we are using the message and params. If a developer doesn't correctly match the number of formatting directives in the message, it will panic when Error is called.

We can do better :-)

First, we should have a New method as that is much safer than manually defining the structs.

Second we should make the params more dynamic.

New Method

The new method should be something like this.

// We should also add some type safety here
type Code string

func New(code Code, msg string) (Error, error) {
    e := Error{Code: code, Message: msg}

    err = e.validate()
        if err != nil {
               return nil, err
        }
    return e, nil
}

func (e *Error) validate() error {
        defer func() {
            if r := recover(); r != nil {
                return fmt.Errorf("invalid Error: %v",r)
            }
        }()
        _ = e.Error()
        return nil
}

Dynamic Params

Instead of relying on a fixed formatted method, we could allow for dynamic params by expecting a key value pair.

func (e *Error) WithParams(params ...interface{}) {
    if len(params)%2 != 0 {
                // Using panic for simplicity of example, but this is a discussion point as to how to handle the error
        panic("Error: Odd number of parameters, expected key-value pairs")
    }
        e.Params = append(e.Params, params...)
        return e
}

func (e *Error) Error() string {
    msg := e.Message
    for i := 0; i < len(e.Params); i += 2 {
        key := params[i]
        value := params[i+1]
        msg = fmt.Sprintf("%s; %v: %v",msg, key, value)
    }
    if e.Err != nil {
        return fmt.Sprintf("%s; error: %v", msg, e.Err)
    }
    return msg
}
mojtaba-esk commented 4 months ago

Saw your comment about creating an issue a bit late, I have applied those on the original PR.

MSevey commented 4 months ago

Saw your comment about creating an issue a bit late, I have applied those on the original PR.

Thanks for adding the New method, that is a good start. I think there are still other parts of this issue that we should discuss. Lower priority though.

smuu commented 2 months ago

done