Add serde support #102

0xf4lc0n · 2023-07-05T13:57:32Z

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.

wiktorwieclaw · 2023-07-07T16:03:05Z

src/parser.rs

+    #[cfg_attr(feature = "serde", serde(serialize_with = "serialize_deque"))]
+    #[cfg_attr(feature = "serde", serde(deserialize_with = "deserialize_deque"))]


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(...) { ... } }

https://serde.rs/field-attrs.html#with

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.

wiktorwieclaw · 2023-07-07T16:13:14Z

src/parser.rs

+        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");


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

Example

wiktorwieclaw · 2023-07-07T16:14:04Z

src/parser.rs

+    type Value = Deque<Vec<Option<Satellite>, 4>, 15>;
+
+    fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+        formatter.write_str("deque")


you should be a bit more specific here

wiktorwieclaw · 2023-07-07T16:14:23Z

Cargo.toml

+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"}


please make sure to fix the formatting

…k error, use more specified 'expecting' message

elpiel · 2023-07-11T08:03:09Z

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

elpiel

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

src/sentences/ztg.rs

elpiel · 2023-07-11T09:19:55Z

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

codecov · 2023-07-20T08:11:15Z

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.

elpiel · 2023-07-20T08:11:25Z

Cargo.toml

@@ -47,6 +48,14 @@ criterion = "0.4"
 [features]
 default = ["std", "all-sentences"]
 std = ["nom/std", "chrono/std", "arrayvec/std"]


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

elpiel · 2023-07-20T08:49:03Z

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

grupacosmo#1

Signed-off-by: Lachezar Lechev <[email protected]>

0xf4lc0n added 10 commits May 23, 2023 23:56

feat: implement serialize trait for structs

da45515

feat!: add deserialization, add serde feature

abde839

fix: use workaround for serde_as behind optional feature

089ace4

chore: activate optional dependencies only by features

2ff00d3

feat: sync with main branch

9828f42

style: make cargo-fmt happy

62eb5c1

feat: use fmt and marker core

3ba5480

chore: remove serde from default features

6a76707

refactor: make clippy happy

254dee2

feat: do not use PhantomData

e308156

wiktorwieclaw reviewed Jul 7, 2023

View reviewed changes

wiktorwieclaw suggested changes Jul 7, 2023

View reviewed changes

0xf4lc0n added 2 commits July 7, 2023 18:53

style: fix formatting

0f63995

feat: move deque serializing/deserializing to mod, propagate push_bac…

7ce11dd

…k error, use more specified 'expecting' message

elpiel reviewed Jul 11, 2023

View reviewed changes

src/sentences/zfo.rs Outdated Show resolved Hide resolved

src/sentences/ztg.rs Outdated Show resolved Hide resolved

0xf4lc0n added 2 commits July 12, 2023 09:48

feat: use DurationSecondsWithFrac instead of DurationSeconds

29dacae

doc: add serde to features list

411fc52

elpiel approved these changes Jul 20, 2023

View reviewed changes

elpiel reviewed Jul 20, 2023

View reviewed changes

elpiel mentioned this pull request Jul 20, 2023

Formatting/serialization support #100

Open

5 tasks

chore: bump MSRV and include std features

e5f9bc9

Signed-off-by: Lachezar Lechev <[email protected]>

elpiel merged commit c7ba32e into AeroRust:main Jul 20, 2023

elpiel mentioned this pull request Jan 25, 2024

Support defmt #92

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add serde support #102

Add serde support #102

0xf4lc0n commented Jul 5, 2023

wiktorwieclaw Jul 7, 2023

wiktorwieclaw Jul 7, 2023

elpiel Jul 20, 2023

wiktorwieclaw Jul 7, 2023

wiktorwieclaw Jul 7, 2023

wiktorwieclaw Jul 7, 2023

elpiel commented Jul 11, 2023 •

edited

Loading

elpiel left a comment

elpiel commented Jul 11, 2023 •

edited

Loading

codecov bot commented Jul 20, 2023 •

edited

Loading

elpiel Jul 20, 2023 •

edited

Loading

elpiel commented Jul 20, 2023

		#[cfg_attr(feature = "serde", serde(serialize_with = "serialize_deque"))]
		#[cfg_attr(feature = "serde", serde(deserialize_with = "deserialize_deque"))]

	#[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"))]

Add serde support #102

Add serde support #102

Conversation

0xf4lc0n commented Jul 5, 2023

wiktorwieclaw Jul 7, 2023

Choose a reason for hiding this comment

wiktorwieclaw Jul 7, 2023

Choose a reason for hiding this comment

elpiel Jul 20, 2023

Choose a reason for hiding this comment

wiktorwieclaw Jul 7, 2023

Choose a reason for hiding this comment

wiktorwieclaw Jul 7, 2023

Choose a reason for hiding this comment

wiktorwieclaw Jul 7, 2023

Choose a reason for hiding this comment

elpiel commented Jul 11, 2023 • edited Loading

elpiel left a comment

Choose a reason for hiding this comment

elpiel commented Jul 11, 2023 • edited Loading

codecov bot commented Jul 20, 2023 • edited Loading

Codecov Report

elpiel Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

elpiel commented Jul 20, 2023

elpiel commented Jul 11, 2023 •

edited

Loading

elpiel commented Jul 11, 2023 •

edited

Loading

codecov bot commented Jul 20, 2023 •

edited

Loading

elpiel Jul 20, 2023 •

edited

Loading