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

Major Refactor #13

Merged
merged 35 commits into from
Mar 21, 2018
Merged

Major Refactor #13

merged 35 commits into from
Mar 21, 2018

Conversation

icholy
Copy link
Collaborator

@icholy icholy commented Jan 18, 2018

This is a pretty big refactor that introduce breaking changes. Let me know what you think.

Breaking changes

  • Add Date & Time types.
  • Convert numeric fields to either float64 or int64.
  • Rename Sentence to Sent
  • Rename Sent.GetSentence() to Sent.Sentence()
  • Rename SentenceI to Message
  • Rename NewLatLong to ParseLatLong

Non-Breaking changes

  • Add ParseSentence(string)
  • Add Sent.Prefix() string
  • Add nmea: prefix to error messages.
  • Unify error message format.
  • Add parser to simplify sentence parsing.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.0%) to 90.081% when pulling 28e43e6 on icholy:master into c227bc3 on adrianmo:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+3.0%) to 90.081% when pulling 28e43e6 on icholy:master into c227bc3 on adrianmo:master.

@coveralls
Copy link

coveralls commented Jan 18, 2018

Coverage Status

Coverage increased (+3.4%) to 90.486% when pulling f4a1466 on icholy:master into c227bc3 on adrianmo:master.

@coveralls
Copy link

coveralls commented Jan 18, 2018

Coverage Status

Coverage increased (+2.4%) to 89.504% when pulling b310829 on icholy:master into c227bc3 on adrianmo:master.

@coveralls
Copy link

coveralls commented Jan 19, 2018

Coverage Status

Coverage increased (+3.7%) to 90.76% when pulling 94fb2ae on icholy:master into c227bc3 on adrianmo:master.

@coveralls
Copy link

coveralls commented Jan 19, 2018

Coverage Status

Coverage increased (+3.9%) to 90.965% when pulling d1efd26 on icholy:master into c227bc3 on adrianmo:master.

@coveralls
Copy link

coveralls commented Jan 19, 2018

Coverage Status

Coverage increased (+9.6%) to 96.721% when pulling 7a45390 on icholy:master into c227bc3 on adrianmo:master.

parser.go Outdated
return p.err
}

// SetErr assigns the an error. Calling this method has no

Choose a reason for hiding this comment

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

Fix typo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bogdanprodanj I doubt this is going to get merged.

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.

@icholy thank you very much for the time you took to improve the library. I really like your changes, specially, the way you have simplified and unified the parsing process. But before merging it, I would like to ask you the following:

  • provide a parser_test.go to cover the new parser.go generic parser helper.
  • provide coverage for the new lines introduced in sentence.go and types.go. Exact lines here and here marked as "NEW".
  • fix the golint offenses. It's not breaking the tests because we need to add the new -set_exit_status flag to the golint command in .travis.yml. Can you add that too? :)
  • fix the typo mentioned by @bogdanprodanj

Thanks again. I really appreciate your work.

@icholy
Copy link
Collaborator Author

icholy commented Mar 15, 2018

@adrianmo I didn't notice your message till right now. I'll update the pr asap.

@adrianmo
Copy link
Owner

Great @icholy :)

@icholy
Copy link
Collaborator Author

icholy commented Mar 16, 2018

@adrianmo ready for review, bumped your coverage by 10% ;). I skipped the test for Sentence.Validate, I don't think it makes sense to test a function that returns nil. But if you really want me to, let me know and I'll add it.

@adrianmo
Copy link
Owner

@icholy thank you for making the changes. I'm merging this PR now.

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

4 participants