Closed aldas closed 1 year ago
Looks like it's time for a v2.
Just a note Github flow uses go get -u golang.org/x/lint/golint@latest
but from Go 1.18 this way of installing commands is removed and go install golang.org/x/lint/golint@latest
should be used. But go install
is supported only from Go 1.16. Maybe just drop 1.15 and 1.14 from github flow as these versions have been EOL long time now.
Yep, I agree.
This is how I imagine the API:
p := nmea.SentenceParser{
ParsePrefix: func(prefix string) (string, string, err) {
if prefix == "GSENSORD" {
return "", prefix, nil
}
return nmea.ParsePrefix(prefix)
},
CheckCRC: func(s nmea.BaseSentence, fields string) error {
if s.Type == "GSENSORD" {
return nil
}
return nmea.CheckCRC(s, fields)
},
}
p.Register("GSENSORD", func(s nmea.BaseSentence) (nmea.Sentence, error) {
// ...
})
The zero value nmea.SentenceParser
is valid.
var p nmea.SentenceParser
_, _ = p.Parse("...")
At the moment I would suggest
p := nmea.NewSentenceParserWithConfig(nmea.SentenceParserConfig{
CustomParsers: map[string]nmea.ParserFunc{
"XYZ": func(s nmea.BaseSentence) (nmea.Sentence, error) {
// This example uses the package builtin parsing helpers
// you can implement your own parsing logic also
p := nmea.NewParser(s)
return XYZType{
BaseSentence: s,
Time: p.Time(0, "time"),
Label: p.String(1, "label"),
Counter: p.Int64(2, "counter"),
Value: p.Float64(3, "value"),
}, p.Err()
},
},
})
s, err := p.Parse(`$00XYZ,220516,A,23,5133.82,W*42`)
p := nmea.NewSentenceParserWithConfig(nmea.SentenceParserConfig{
is way longer (at least first part) but by creating instance with function you no not need to ever argue with anyone, should it be co-routine safe or not. This aligns with global sentence parser as it seems also be co-routine safe in regards of custom parsers. This PR introduces these new additional configurable callbacks that could also be potentially be racy.
My experience with opensource libraries is the if there is a way - then someone will shoot them in the foot and very few read comments/warning on methods/functions whether something is or is not co-routine safe.
// SentenceParser provides configurable functionality to parse raw input into Sentence
type SentenceParser struct {
// parserLock exists because we want to preserve defaultSentenceParser backwards compatibility so global
// RegisterParser and MustRegisterParser functions can mutate customParsers map. SentenceParser instance itself
// does not need locks
parserLock *sync.RWMutex
config SentenceParserConfig
}
// NewSentenceParser creates new default instance of SentenceParser
func NewSentenceParser() *SentenceParser {
return NewSentenceParserWithConfig(SentenceParserConfig{})
}
// NewSentenceParserWithConfig creates new instance of SentenceParser with given config
func NewSentenceParserWithConfig(c SentenceParserConfig) *SentenceParser {
if c.ParseAddress == nil {
c.ParseAddress = DefaultParseAddress
}
if c.CheckCRC == nil {
c.CheckCRC = DefaultCheckCRC
}
cp := map[string]ParserFunc{}
for sType, pFunc := range c.CustomParsers {
cp[sType] = pFunc
}
return &SentenceParser{
parserLock: &sync.RWMutex{},
config: SentenceParserConfig{
CustomParsers: cp,
ParseAddress: c.ParseAddress,
CheckCRC: c.CheckCRC,
OnTagBlock: c.OnTagBlock,
},
}
}
p := nmea.NewSentenceParserWithConfig(nmea.SentenceParserConfig{ is way longer (at least first part) but by creating instance with function you no not need to ever argue with anyone, should it be co-routine safe or not. This aligns with global sentence parser as it seems also be co-routine safe in regards of custom parsers.
The top level functions are thread safe because it's likely that code in different packages/modules use them without being aware of each-other. If a package is creating its own nmea.SentenceParser
then it can guarantee that no-one else is using it. In general, types are not thread safe by default because it adds unnecessary overhead and the user can lock around it if they need to.
This PR introduces these new additional configurable callbacks that could also be potentially be racy.
How would the suggested lock prevent races in the user-provider functions?
My experience with opensource libraries is the if there is a way - then someone will shoot them in the foot and very few read comments/warning on methods/functions whether something is or is not co-routine safe.
Then let them shoot themselves in the foot. Maybe they'll learn to read the docs.
alright, this should be better now. Changed mutex parts + SenteceParser
is plain struct with public fields. MTK
changes are resolved as - global Parse work with old MTK
parser and when SentenceParser
instance is created - it does not contain old MTK
parser and works with newer PMTK001
Ack parser.
If it comes to merging some day - please use Squash or allow me to squash commits
@aldas feel free to merge if you're ready. @adrianmo FYI
SentenceParser
struct) - See #96SupportedParsers
),BREAKING CHANGE:
I have added MTK change here as it is deeply tied to sentence parsing internals and has "hacks" to override BaseSentence.TalkerID meaning.