Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: add callback interface to parse #48

Merged
merged 1 commit into from
Mar 19, 2019
Merged

Conversation

tkurki
Copy link
Collaborator

@tkurki tkurki commented Mar 17, 2019

If a client app wants to keep of track of what output
was generated from certain input an event based interface
is less than ideal, so add an optional callback to parse().

I have some multiplexed SK server log files where the timestamp in n2k data is bad. I would like to change autodetect/canboat provider in @signalk/streams so that the timestamp from the multiplexed log overrides the one from the actisense serial data. For that I need to catch
parse output in CanboatJs.prototype._transform and override timestamp there.

Overall with streams and backpressure handling it is much better to handle processing with callbacks instead of separate events. Now calling done without any knowledge of what gets pushed by the data we are handling in CanboatJs.prototype._transform is sort of recipe for disaster in the future, even if it works now.

@tkurki
Copy link
Collaborator Author

tkurki commented Mar 17, 2019

Thoughts? As the parsing is synchronous we could just as easily return the parsed value and throw Errors.

@sbender9
Copy link
Member

I'm fine with changing to returning/throwing, but I think we have to leave it like this because of the way the dependency in node server will pick the new version.

@tkurki
Copy link
Collaborator Author

tkurki commented Mar 18, 2019

Yep, this way it is backwards compatible. I'll see about adding some tests.

If a client app wants to keep of track of what output
was generated from certain input an event based interface
is less than ideal, so add an optional callback to parse().
@tkurki
Copy link
Collaborator Author

tkurki commented Mar 19, 2019

Now there's a happy path test, but I have trouble adding more test coverage. Like how do I cause errors? There's now a commented out test for calling the callback with errors.

The functionality itself is done.

How do you want to proceed?

@sbender9 sbender9 merged commit 260d1e3 into master Mar 19, 2019
@tkurki
Copy link
Collaborator Author

tkurki commented Mar 19, 2019

Yep, no problem with mocha. I was vauge - I was thinking more along the lines

  • is there an easy way to tailor input that would trigger the error callback?
  • there are no tests for parseBuffer and parsePgnData, where I also added the callback. Is there an easy way to generate input to test those?
  • anything else I should be aware of?

Maybe I'm thinking too much here - I'm ok with merging it as it is.

@sbender9 sbender9 deleted the parse-callback branch March 31, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants