adrianmo / go-nmea

A NMEA parser library in pure Go
MIT License
226 stars 77 forks source link

parsing errors #89

Closed aldas closed 2 years ago

aldas commented 2 years ago

About errors. When parsing fails currently only first error is stored and all subsequent errors are discarded. I propose if p.EnumString and other parsing method errors would be distinguishable from each other and parsers would store all errors. it would allow you to introspect what went wrong and some circumstances ignore some error types and still use the parsed sentence.

maybe just wrapping them (or storing into slice or multierror)

func (p *Parser) SetErr(context, value string) {
    if p.err == nil {
        p.err = fmt.Errorf("nmea: %s invalid %s: %s", p.Prefix(), context, value)
    } else {
        p.err = fmt.Errorf("nmea: %s invalid %s: %s, err: %w", p.Prefix(), context, value, p.err)
    }
}
icholy commented 2 years ago

I don't like the error wrapping approach because it will break all the existing tests and the error messages will be very ugly. The slice/multi-error approach will let use keep backwards compatibility. However, I'd like to understand the "why" a bit more. Can you describe a more specific use case?

maybe something like this?:

type UnsupportedError struct {
    Raw    string
    Prefix string
}

func (e UnsupportedError) Error() string {
    return fmt.Sprintf("nmea: invalid prefix: %s", e.Prefix)
}

type FieldError struct {
    Context string
    Value   string
}

func (e FieldError) Error() string {
    return fmt.Sprintf("nmea: invalid %s: %s", e.Context, e.Value)
}

type ParseError struct {
    Raw    string
    Errors []error
}

func (e ParseError) Error() string {
    if len(e.Errors) == 0 {
        panic("nmea: empty error")
    }
    return e.Errors[0].Error()
}
aldas commented 2 years ago

when parsing fails it stops at first problem. which means that if there are more than 1 field problematic I would need to parse sentence multiple times to know what all problems are. In production environment this is impossible or tedious - especially if you are doing first assessment of the problem scope.


in certain situation when you use library just as a simple converter-gateway to another system (ala serial reader -> nmea parser -> mqtt -> ...) you want to delegate decision making from library to another component downstream. It is not possible to make decision if sentence that was parsed has incomplete values because parsing actually stopped after first error and relevant information for you is actually ok. For example - some enum fields can contain values that go-nmea does not recognize. I know from reading device manuals that sometimes manufacturers only document enum values that their device uses and left out values that are specified by NMEA. Sometimes values change - ala new GNSS constellations are launched. I think 15 year ago there were 4 constellations - now there are 6. Although gnss constellations are not enums, but there are cases where constellation is certain place at value for example GNS.Mode field. Let alone obscure devices that send their custom values. I think there are already reported issues that there are devices that do not send CRC. Everything is easy when software is meant to use specific hardware - but situation where devices vary it gets harder as you can not predict what devices you are using in future.

Currently parsing methods return early when parser has already registered an error

func (p *Parser) Int64(i int, context string) int64 {
    s := p.String(i, context)
    if p.err != nil {
        return 0
    }

but maybe this could be made "optional" by some flag or default not to stop.

icholy commented 2 years ago

Ok, that makes sense to me. If you want to put together a PR I'll review. I don't think we need a flag to disable the short circuiting. We can just remove it without breaking anything.