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

Adds MWV and MDA parsers #63

Merged
merged 5 commits into from
Dec 20, 2022
Merged

Conversation

AndrewLipscomb
Copy link

Adds a pair of parsers for WIMDA and WIMWV. No GPS Fix related messaging parsing for this set as they are both weather station based messages.

@elpiel
Copy link
Member

elpiel commented Nov 7, 2022

Thank you for working on this! I will make sure to review the PR in the coming days.

@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Base: 79.50% // Head: 79.50% // No change to project coverage 👍

Coverage data is based on head (379d7a2) compared to base (379d7a2).
Patch has no changes to coverable lines.

❗ Current head 379d7a2 differs from pull request most recent head a50f483. Consider uploading reports for the commit a50f483 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #63   +/-   ##
=======================================
  Coverage   79.50%   79.50%           
=======================================
  Files          22       22           
  Lines         854      854           
=======================================
  Hits          679      679           
  Misses        175      175           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

The PR looks great so far. There are a few tweaks and small changes + fixing the CI build but other than that great contribution <3 !

Would you mind adding both example sentences to the test case all_supported_messages too?

src/sentences/mda.rs Outdated Show resolved Hide resolved
src/sentences/mda.rs Outdated Show resolved Hide resolved
src/sentences/mda.rs Show resolved Hide resolved
src/sentences/mwv.rs Outdated Show resolved Hide resolved
src/sentences/mwv.rs Outdated Show resolved Hide resolved
src/sentences/mwv.rs Outdated Show resolved Hide resolved
@AndrewLipscomb
Copy link
Author

AndrewLipscomb commented Nov 26, 2022

Alrighty - still getting the hang of Rust's docs and linting, but cargo fmt and cargo clippy seem like happy bois now. Added the test case too

Edit - forgot the pub members on MDA - fixed now. Give it a whirl Shirl

Copy link

@clemarescx clemarescx left a comment

Choose a reason for hiding this comment

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

Looks good to me, just the nitpick about using preceded(char(','), <...>) instead of let (i, _) = char(',')(i)?; in the MWV parser


fn do_parse_mwv(i: &str) -> IResult<&str, MwvData> {
let (i, direction) = opt(float)(i)?;
let (i, reference_type) = opt(preceded(char(','), one_of("RT")))(i)?;

Choose a reason for hiding this comment

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

To handle the comma separators, you should parse each field separated with let (i, _) = char(',')(i)?; instead of using preceded(char(','), <...>)(i)?; when parsing the field itself, like you did in the whole MDA parser.
This is not a big deal, mostly for the sake of consistency with the rest of the crate.
According to the benchmarks (run cargo bench in the project), neither seem to outperform the other in a significant way.

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.

I'm good with merging this PR as it is and doing additional changes onto it.

One task remains too (for reference):

  • Add both example sentences to the test case all_supported_messages

@elpiel elpiel merged commit 12b6615 into AeroRust:main Dec 20, 2022
@elpiel elpiel linked an issue Jan 24, 2023 that may be closed by this pull request
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.

Supporting additional sentences
3 participants