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

Add serde support #102

Merged
merged 15 commits into from
Jul 20, 2023
Merged

Add serde support #102

merged 15 commits into from
Jul 20, 2023

Conversation

0xf4lc0n
Copy link

@0xf4lc0n 0xf4lc0n commented Jul 5, 2023

This PR adds support for serialization and deserialization via serde as requested in #100.

Because SatsPack uses Deque which doesn't derive Serialize and Deserialize traits, a custom serialization/deserialization functions were created.

src/parser.rs Outdated
Comment on lines 450 to 451
#[cfg_attr(feature = "serde", serde(serialize_with = "serialize_deque"))]
#[cfg_attr(feature = "serde", serde(deserialize_with = "deserialize_deque"))]

Choose a reason for hiding this comment

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

Suggested change
#[cfg_attr(feature = "serde", serde(serialize_with = "serialize_deque"))]
#[cfg_attr(feature = "serde", serde(deserialize_with = "deserialize_deque"))]
#[cfg_attr(feature = "serde", serde(with = "serde_deque"))]
mod serde_deque {
    fn serialize(...) { ... }
    fn deserialize(...) { ... }
}

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for reviewing this PR @wiktorwieclaw . Your comment has been updated in the PR. We can fit it into 1 like but I have approved the PR as is.

src/parser.rs Outdated
let mut deq: Deque<Vec<Option<Satellite>, 4>, 15> = Deque::new();

while let Some(v) = seq.next_element()? {
deq.push_back(v).expect("Cannot deserialize");

Choose a reason for hiding this comment

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

panic is inappropriate, you can early return with an Err case containing an error created using this trait.

Example

src/parser.rs Outdated
type Value = Deque<Vec<Option<Satellite>, 4>, 15>;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("deque")

Choose a reason for hiding this comment

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

you should be a bit more specific here

Cargo.toml Outdated
Comment on lines 29 to 31
arrayvec = { version = "0.7.2", default-features = false}
chrono = { version = "0.4.19", default-features = false }
heapless = "0.7.15"
heapless = { version ="0.7.15"}

Choose a reason for hiding this comment

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

please make sure to fix the formatting

@elpiel
Copy link
Member

elpiel commented Jul 11, 2023

Thank you for the PR @0xf4lc0n!
I will take a look in the coming days.
@wiktorwieclaw also thank you for provided feedback. I already see that your comments have been tackled

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.

Looks good so far.
There are only 2 things I managed to see for the Duration (de)serialization and I think it's good to discuss whether or not we should use camelCase for the field names.

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

elpiel commented Jul 11, 2023

  • Please add the feature to the features list in lib.rs

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.86 ⚠️

Comparison is base (4b9b652) 79.46% compared to head (e5f9bc9) 78.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
- Coverage   79.46%   78.60%   -0.86%     
==========================================
  Files          33       33              
  Lines        1193     1206      +13     
==========================================
  Hits          948      948              
- Misses        245      258      +13     
Impacted Files Coverage Δ
src/lib.rs 0.00% <ø> (ø)
src/parse.rs 73.62% <ø> (ø)
src/parser.rs 75.09% <0.00%> (-3.82%) ⬇️
src/sentences/aam.rs 96.29% <ø> (ø)
src/sentences/alm.rs 87.50% <ø> (ø)
src/sentences/bod.rs 77.27% <ø> (ø)
src/sentences/bwc.rs 90.62% <ø> (ø)
src/sentences/bww.rs 100.00% <ø> (ø)
src/sentences/dbk.rs 89.47% <ø> (ø)
src/sentences/faa_mode.rs 60.86% <ø> (ø)
... and 20 more

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

Cargo.toml Outdated
@@ -47,6 +48,14 @@ criterion = "0.4"
[features]
default = ["std", "all-sentences"]
std = ["nom/std", "chrono/std", "arrayvec/std"]
Copy link
Member

@elpiel elpiel Jul 20, 2023

Choose a reason for hiding this comment

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

Would you mind adding serde?/std and serde_with?/std to the std feature list as well

@elpiel elpiel mentioned this pull request Jul 20, 2023
5 tasks
@elpiel
Copy link
Member

elpiel commented Jul 20, 2023

I've opened this PR to bump the MSRV and update the std features:

grupacosmo#1

Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
@elpiel elpiel merged commit c7ba32e into AeroRust:main Jul 20, 2023
12 of 14 checks passed
@elpiel elpiel mentioned this pull request Jan 25, 2024
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

3 participants