Implement requested changes to Tagblocks

simeonmiteff commented 4 years ago

This PR implements the requested changes in the (now closed) PR adrianmo/go-nmea#59 by @klyve (which adds tag blocks support).

coveralls commented 4 years ago

Coverage remained the same at 100.0% when pulling acffae72d32b8772e65fc6cdd1a261054ca77342 on xerra-eo:master into 9e4de63ef7642b58f8dd3959fe35fa1ee99ad884 on adrianmo:master.

simeonmiteff commented 4 years ago

@adrianmo

Thanks @simeonmiteff taking on the Tag Block work! The changes look good to me, I just have a couple of comments. 1- Can you please extend the test case to check that all Tag Block fields are parsed correctly?

I might be missing something, but I think the existing tests cover all the fields:

This case contains every field and they're all valid: https://github.com/adrianmo/go-nmea/blob/239a27d2cc7174320ce0c920c089024f57266565/tagblock_test.go#L63-L77
These three cases test invalid input for the three int64 fields that are also the only ones that can be invalid: https://github.com/adrianmo/go-nmea/blob/239a27d2cc7174320ce0c920c089024f57266565/tagblock_test.go#L105-L122

2- Can you add the support for Tag Blocks as an item in the Readme's features list? 3- It'd be great if you could also add a minimal example of Tag Block parsing in the Readme's examples section.

Done: 4b3b9d4

icholy commented 4 years ago

@adrianmo I'm not too worried about 100% coverage. I'll improve it later.

adrianmo commented 4 years ago

@icholy sounds good - however I'm not sure why the CI status is not reported correctly to this PR.

icholy commented 4 years ago

@adrianmo LGTM, can you manually restart the CI?

adrianmo commented 4 years ago

I restarted it, and it passed, but the status is not updated in the PR. I'll merge the PR and revisit the CI status update if it continues to fail.

icholy commented 4 years ago

@simeonmiteff thank you for the contribution.

adrianmo / go-nmea

Implement requested changes to Tagblocks #78