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

Adding option to include delimiter in the DelimiterParser transform. #1453

Merged
merged 1 commit into from
Jan 25, 2018
Merged

Adding option to include delimiter in the DelimiterParser transform. #1453

merged 1 commit into from
Jan 25, 2018

Conversation

saboya
Copy link
Contributor

@saboya saboya commented Jan 25, 2018

This PR adds a includeDelimiter boolean option to the DelimiterParser that includes the delimiter in its output.

In the 4.x series, I used the DelimiterParser relying on this, because some of the protocols used in the devices I'm operating with responds with a single delimiter as an ACK. The new transform does not trigger an event when an empty delimiter is sent. I added a new line to the first test to illustrate this, sending a single delimiter to the parser and assuring the Sinon spy is only called twice.

I also added a new test with includeDelimiter: true.

Thanks for you work!

Copy link
Member

@reconbot reconbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

@reconbot
Copy link
Member

Travis takes a really long time for osx builds but since this isn't os specific I'm going to just merge.

@reconbot reconbot merged commit 6a3ab65 into serialport:master Jan 25, 2018
@saboya saboya deleted the include-delimiter branch February 7, 2018 12:00
oteku pushed a commit to Cutii/node-serialport that referenced this pull request Apr 9, 2018
reconbot pushed a commit that referenced this pull request Jul 24, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants