Initial work on XDR parser #228
Conversation
|
Where would the user configure this? The nice thing about plugins is that configuration ui is essentially free. Also are you aware of https://github.com/SignalK/nmea0183-signalk#custom-sentences ? |
|
No, wasn't ware. But, this is a PR to expand the functionality of this parser, not just for users of Signal K server. |
| } | ||
| ] | ||
|
|
||
| module.exports = function (input) { |
There was a problem hiding this comment.
I'd really like to have the parser support injection of XDR configuration in constructor. Hooks are stateless, so what if we add a third parameter to the parse call. Here it would look like
module.exports = function (input, session, options) {
and options would be the hook specific options, in XDR case the dictionary.
Another options would be to modify hook loading that a hook could export a constructor with parameters that returns the actual function. This would allow validating the expressions once on load. Now a malformed expression will remain active and probably? log an error for each parse attempt.
There was a problem hiding this comment.
I'll look into this, good suggestion
| continue | ||
| } | ||
|
|
||
| const def = definitions.find(({ name }) => (name === boundary)); |
There was a problem hiding this comment.
If there is ever going to be only one active definition per name, so why not make definitions an object? Then this would be definitions[name].
The current structure doesn't support having multiple formulas for multiple units. Another object layer keyed by unit would allow definitions[name][unit] lookup, supporting multiple units per definition.
There was a problem hiding this comment.
This behaviour is mostly a result of me attempting to re-use the definitions from xdr-parser-plugin, which became less-and-less tenable as I went through development. All good points.
| fill = true; | ||
| } | ||
|
|
||
| if (fill === false || p === boundary) { |
|
|
||
| const math = require('math-expression-evaluator'); | ||
| const fs = require('fs'); | ||
| const xdrDictionary = { definitions: [] }; |
There was a problem hiding this comment.
What's the idea with having definitions and not just definitions directly under root?
There was a problem hiding this comment.
See previous comment regarding xdr-parser-plugin
| continue; | ||
| } | ||
|
|
||
| if (def.units !== unit) { |
There was a problem hiding this comment.
I think both should be in singular or plural.
There was a problem hiding this comment.
See previous comment regarding xdr-parser-plugin
|
|
||
| try { | ||
| // Populate a dictionary | ||
| const xdrDictPath = require.resolve('xdr-parser-plugin/xdrDict'); |
There was a problem hiding this comment.
I am not terribly fond of having a configuration file that is loaded using require.resolve. I think configuration should be injected from the context that uses the parser. We can support both though if you think this is the way to go, just that we should not log a warning if there is no configuration file.
There was a problem hiding this comment.
See previous comment regarding xdr-parser-plugin
| 'navigation.attitude.roll' | ||
| ) | ||
| delta.updates[0].values[1].value.should.be.closeTo(0.0087, 0.00005) | ||
| }) |
There was a problem hiding this comment.
How about some non happy path tests - a malformed expression, input with no matching definition?
|
@tkurki thanks for review, I'll update. I'd like to ship with a slightly more complete set of defs if possible, so if you have any XDR data (that's not already in the repo of signalk-server), please let me know. |
This PR implements an initial implementation for an XDR parser that uses the definitions format from
xdr-parser-plugin. The implementation is different however, as that plugin has several issues IMO (e.g.requirestatements within function bodies).In order to finish this PR, it would make sense (to me) to internalise the dictionary of XDR sentences rather tha pulling it from
xdr-parser-plugin, and expanding on the definitions in there. Also, it would make sense to make the expression dependent on the unit (RN I'm simply checking if the units match).