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

Adding WNC sentence type #110

Merged
merged 12 commits into from
Dec 21, 2023
Merged

Adding WNC sentence type #110

merged 12 commits into from
Dec 21, 2023

Conversation

jarrod817
Copy link
Contributor

No description provided.

@jarrod817
Copy link
Contributor Author

I would like to propose adding the WNC sentence type.

src/sentences/wnc.rs Outdated Show resolved Hide resolved
Copy link
Member

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

Only a minor thing regarding the docs and I don't have any feedback on the actual implementation.

Once again thank you for you contribution!

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (4e10c12) 77.51% compared to head (099bd4c) 77.62%.

Files Patch % Lines
src/sentences/wnc.rs 85.71% 3 Missing ⚠️
src/parser.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
+ Coverage   77.51%   77.62%   +0.11%     
==========================================
  Files          33       34       +1     
  Lines        1223     1247      +24     
==========================================
+ Hits          948      968      +20     
- Misses        275      279       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor Author

@jarrod817 jarrod817 left a comment

Choose a reason for hiding this comment

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

Added documentation

Copy link
Member

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

Ok, so I took a look again this morning and I also remembered a few things.
Please add these things to the PR:

  • A feature for the sentence to be enabled in Cargo.toml (enabled for waypoint feature too)
  • add the example sentence in all_supported_messages.rs test

I've also suggested changes to the docs to resemble other places. I do wish to make the docs consistent across the crate eventually.

src/sentences/wnc.rs Outdated Show resolved Hide resolved
src/sentences/wnc.rs Outdated Show resolved Hide resolved
@elpiel
Copy link
Member

elpiel commented Nov 23, 2023

I've also fixed the MSRV and all the warnings so you should be good to go if your code follows rustfmt/clippy.

src/parse.rs Outdated Show resolved Hide resolved
@elpiel elpiel merged commit 749ebf6 into AeroRust:main Dec 21, 2023
14 checks passed
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

2 participants