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

Check sentence starts with dollar ($) sign #7

Merged
merged 2 commits into from
Jun 5, 2017
Merged

Check sentence starts with dollar ($) sign #7

merged 2 commits into from
Jun 5, 2017

Conversation

bezineb5
Copy link
Contributor

@bezineb5 bezineb5 commented Jun 2, 2017

Actually, my issue is that I have some malformed NMEA sentences due to serial communication issues, which led to a crash in the library. My intention here is simply to avoid the crash, not necessarily to recover from it.
However, reading a bit about NMEA, it seems that $ is a "start delimiter", so I agree with your solution to start reading from the last $.
I'm checking the * after splitting.
I also added some tests.

@coveralls
Copy link

coveralls commented Jun 2, 2017

Coverage Status

Coverage increased (+0.1%) to 86.719% when pulling 8410419 on bezineb5:fix-start into 523439f on adrianmo:master.

@adrianmo
Copy link
Owner

adrianmo commented Jun 2, 2017

@bezineb5 I've been looking at it a bit deeper and I rethinking about it. Here's my opinion:

This lib is a parser that takes an input text, parses it, and gives you back a NMEA Sentence object if the text is a valid sentence. In case the input text is not a valid sentence, the lib returns an error. Just like any JSON or YAML parses out there when you feed them with incorrectly formatted text. IMO the lib should not attempt to fix any input text as it can lead to thinking that the input text was correctly formatted. Fixing the input text should be a pre-processing that belongs to the app importing and using this lib.

I've been looking at a couple of NMEA parsers and they seem to work like this.

Let me know what you think.

@bezineb5
Copy link
Contributor Author

bezineb5 commented Jun 2, 2017

I agree. It makes the behavior less predictable if the lib tries to fix things. And this kind of case should only happen if there is another issue, such as a connection problem. I'll update the PR to enforce $ at the beginning, and make sure that it fails correctly with improper inputs.

@adrianmo
Copy link
Owner

adrianmo commented Jun 2, 2017

Sounds good. Instead of just checking that the sentence contains $, we could check the index is 0.

@coveralls
Copy link

coveralls commented Jun 2, 2017

Coverage Status

Coverage increased (+0.1%) to 86.719% when pulling 0e1dfa8 on bezineb5:fix-start into 523439f on adrianmo:master.

Copy link
Owner

@adrianmo adrianmo left a comment

Choose a reason for hiding this comment

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

@bezineb5 the changes look good. I'm going to update the PR title and merge it. Thanks!

@adrianmo adrianmo changed the title Starts reading the sentence from the last $ sign, even if it is not a… Check sentence starts with dollar ($) sign Jun 5, 2017
@adrianmo adrianmo merged commit adc20ca into adrianmo:master Jun 5, 2017
@bezineb5 bezineb5 deleted the fix-start branch June 6, 2017 11:09
@bezineb5
Copy link
Contributor Author

bezineb5 commented Jun 6, 2017

Thanks to you!

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.

None yet

3 participants