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

Add an option for ignoring wrong checksum #86

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

roseshahar
Copy link

I want to add an option for not raising an exception when your try to parse with wrong checksum

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.057% when pulling bd68950 on shaharrose:feature/parse-ignore-wrong-checksum into c4fc66c on Knio:master.

@xOneca
Copy link
Contributor

xOneca commented Mar 29, 2019

Why not use the already existing check parameter (still fixing the logic)?

@roseshahar
Copy link
Author

Why not use the already existing check parameter (still fixing the logic)?

@xOneca Because the check requires the NMEA sentence to have checksum in it, and I feel that allowing wrong checksum needs a parameter of its own

@xOneca
Copy link
Contributor

xOneca commented Mar 29, 2019

Allowing wrong checksum is not checking it, isn't it?

I mean, it seems to me that the original check parameter should be doing what you want to do with the new parameter, but it isn't doing it. I would make check raise an exception only if there's no checksum or the check is wrong.

@roseshahar
Copy link
Author

@xOneca I think that the check argument is really weird as well, but I think that @Knio should agree to the change of the check purpose

@xOneca
Copy link
Contributor

xOneca commented Mar 29, 2019

Sure.

@fohrloop
Copy link

fohrloop commented Jul 7, 2020

I think this is fixed in a pull request I just made: #115

@xOneca
Copy link
Contributor

xOneca commented Jul 9, 2020

It feels to me more correct the fix in 91204af from @np-8.

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.

4 participants