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

Support for GNGNS Strings #42

Merged
merged 9 commits into from
Dec 19, 2018
Merged

Support for GNGNS Strings #42

merged 9 commits into from
Dec 19, 2018

Conversation

bmurray
Copy link
Contributor

@bmurray bmurray commented Dec 16, 2018

I also rolled in a change to allow SNR values to be Floats. I have a receiver that always reports SNR with a float value.

Allowed Floats for GPGSV and GLGSV
@coveralls
Copy link

coveralls commented Dec 16, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling c7d459b on bmurray:master into a32116e on adrianmo:master.

@icholy
Copy link
Collaborator

icholy commented Dec 17, 2018

Considering that the mode values are contextual, I don't see the use-case for the IsMode method.

@icholy
Copy link
Collaborator

icholy commented Dec 18, 2018

@bmurray I'm ready to merge it if you revert back to the original PR (and the tests are passing).

@icholy
Copy link
Collaborator

icholy commented Dec 18, 2018

@adrianmo the change to a float64 will be a breaking change. We'll have to bump the major version :/

Reverted float to int for GSV SNR values
@bmurray
Copy link
Contributor Author

bmurray commented Dec 18, 2018

Thinking more about it, the *GSV values should be Int64, as that's what's in the spec. The library shouldn't compromise for broken chips.

I've also removed the IsMode from the GNGNS string. Test's are all passing on my end.

@icholy
Copy link
Collaborator

icholy commented Dec 19, 2018

@bmurray the CI is failing because everything exported needs to be commented. I'm not the owner, so I can't merge until it's passing.

@bmurray
Copy link
Contributor Author

bmurray commented Dec 19, 2018

Hm, they're commented now, and it runs against current Go. I don't know what those other errors are related to; it seems like Travis-CI is having issues pulling golint ?

@icholy
Copy link
Collaborator

icholy commented Dec 19, 2018

@bmurray yeah, this is related to #41

Test against Go 1.11.x
@bmurray
Copy link
Contributor Author

bmurray commented Dec 19, 2018

golang/lint#421

Looks like 1.11, and 1.10 are the only ones officially supported for lint. Should just drop testing against 1.7 and 1.8; 1.9 still works for now, so that's a bit of a tossup.

@icholy icholy merged commit 0c836e0 into adrianmo:master Dec 19, 2018
@icholy
Copy link
Collaborator

icholy commented Dec 19, 2018

Thanks for the contribution!

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.

3 participants