epoch, duration: improve and fix serdes feature #175

gwbres · 2022-11-15T07:59:49Z

Signed-off-by: Guillaume W. Bres guillaume.bressaix@gmail.com

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>

gwbres · 2022-11-15T08:02:12Z

the serdes traits for Duration and Epoch do not work on my side.
When I try to import these two in structures that need serdes traits, it complains the traits are not implemented.

In the previous code I don't understand why all of these options were depending on "std" instead of "serde".

Timescale does work, I pushed it myself a couple of weeks ago using the auto derivation.
I modified the previous code to use auto derivation. I don't think we need a custom implementation either, the two structures are very straightforward and only comprise native types

ChristopherRabotin

Deriving serde is definitely nicer than the custom implementations I had. Could you add some tests for the new serialization because it seems like I removed those.

ChristopherRabotin · 2022-11-15T15:41:27Z

src/epoch.rs

@@ -159,6 +159,7 @@ const CUMULATIVE_DAYS_FOR_MONTH: [u16; 12] = {
 #[derive(Copy, Clone, Eq)]
 #[repr(C)]
 #[cfg_attr(feature = "python", pyclass)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]


Suggested change

#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]

#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]

ChristopherRabotin · 2022-11-15T15:42:02Z

src/duration.rs

@@ -57,6 +57,7 @@ pub const NANOSECONDS_PER_CENTURY: u64 = DAYS_PER_CENTURY_U64 * NANOSECONDS_PER_
 #[derive(Clone, Copy, Debug, PartialOrd, Eq, Ord)]
 #[repr(C)]
 #[cfg_attr(feature = "python", pyclass)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]


Suggested change

#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]

#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]

Yes! I think that was missing. Also, this issue should have been caught in the tests, but I know that the tests for serde are crappy ... actually did I remove the test? I don't see it right now!

you mean the actual code (1) ? or in the CI logs (2) ?

(1) the code is now replaced by test methods that will get triggered if a CI jobs enables the serde feature.
(2) apparently the previous CI --with-non-default stopped at "std" and did not resolve "std->serde"

ChristopherRabotin · 2022-11-15T15:42:32Z

src/duration.rs

@@ -21,8 +21,8 @@ use core::fmt;
 use core::hash::{Hash, Hasher};
 use core::ops::{Add, AddAssign, Div, Mul, Neg, Sub, SubAssign};

-#[cfg(feature = "std")]
-use serde::{de, Deserialize, Deserializer};
+#[cfg(feature = "serde")]


Suggested change

#[cfg(feature = "serde")]

#[cfg(feature = "std")]

There isn't any "serde" feature in the Cargo.toml so I'm guessing that this line causes the serde imports to never be executed.

From Cargo.toml:

[features] default = ["std"] std = ["serde", "serde_derive"] asn1der = ["der"] python = ["std", "asn1der", "pyo3"]

When the "std" feature is enabled, it should be importing serde, but I think that you've nailed the source of the issue with your change in Duration (I'll write another comment).

Hello Chris

the following only works when the feature bears a crate exact name.

See my rinex Cargo.tml

"flate2" and "serde" are two features, apparently no declaration, but this is unlocked with "optionnal=true" when summoning these libraries.
Set your default behavior in [features] and they behavior in this case (this is important).
Set whatever features they might come with, for instance "derive" for "serdes" is rapidly needed.

See the "with-geo" feature I declared too, which is the combination of two packages, and I don't know how to manage this case, so I declared a "custom" feature

Well I'll add one thing.
With your current Cargo.toml "Serde::Serialize" and "Deseriliaze" are imported by "std": that answers my initial question.

Removing these two would actually work too, with my previous explanation

Wow, I had no idea! I've been developing in Rust since 2017 but I learn new things regularly. Thanks

Same thing here..

rust_docs clearly lacks some capabilities regarding crate features. They are totally undocumented in the API. Like there is absolutely no mean to know what is available besides reading the Cargo.toml..

There is a rust crate that allows documenting a crate feature, it parses documentation off Cargo.toml, so it will grow a little too much IMO. And it's also one more dependency required for your crate. I think they should come with a solution to this problem in a near future

ChristopherRabotin · 2022-11-15T15:42:49Z

src/epoch.rs

@@ -28,8 +28,8 @@ use pyo3::prelude::*;
 #[cfg(feature = "python")]
 use pyo3::pyclass::CompareOp;

-#[cfg(feature = "std")]
-use serde::{de, Deserialize, Deserializer};
+#[cfg(feature = "serde")]


Suggested change

#[cfg(feature = "serde")]

#[cfg(feature = "std")]

so in conclusion, "serde" already exists in the current Cargo.toml
and it's usually invoked with "serde"

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>

codecov-commenter · 2022-11-15T17:33:11Z

Codecov Report

Base: 78.57% // Head: 79.26% // Increases project coverage by +0.68% 🎉

Coverage data is based on head (82593be) compared to base (e44444e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   78.57%   79.26%   +0.68%     
==========================================
  Files           9        9              
  Lines        2586     2580       -6     
==========================================
+ Hits         2032     2045      +13     
+ Misses        554      535      -19

Impacted Files	Coverage Δ
src/duration.rs	`85.55% <100.00%> (+1.65%)`	⬆️
src/epoch.rs	`86.82% <100.00%> (+0.47%)`	⬆️
src/timescale.rs	`93.90% <100.00%> (+3.36%)`	⬆️

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.

ChristopherRabotin · 2022-11-15T19:15:58Z

.github/workflows/tests.yml

@@ -61,6 +61,9 @@ jobs:

      - name: Test (no default features)
        run: cargo test --no-default-features
+
+      - name: Test (all features)
+        run: cargo test --all-features


The python feature can only be built with the PyO3 tools. So this line needs to be all of the features, minus the python feature.

Apparently --no-default-features only tests "std" (that is my understanding)
Maybe we should create a dedicated step like "ANS1" ? I don't know

Yes, that would also work

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>

Signed-off-by: Christopher Rabotin <christopher.rabotin@gmail.com>

ChristopherRabotin · 2022-11-16T07:29:35Z

Merging in a minute, thanks for that fix.

Let me know when you need a release and I'll update the change log, make a tag, and publish.

epoch, duration: improve and fix serdes feature

c6a6fce

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>

ChristopherRabotin approved these changes Nov 15, 2022

View reviewed changes

gwbres added 5 commits November 15, 2022 18:25

epoch: test serdes ops

3f8c96c

github workflow: add test with all features

87df2db

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>

duration: add serdes test

3721627

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>

timescale: add serdes test

cdc0dba

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>

epoch: remove deprecated serde test

ef829bf

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>

ChristopherRabotin approved these changes Nov 15, 2022

View reviewed changes

ChristopherRabotin reviewed Nov 15, 2022

View reviewed changes

gwbres and others added 2 commits November 15, 2022 20:42

github workflow: test serde features

cebaf5e

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>

Cargo fmt

82593be

Signed-off-by: Christopher Rabotin <christopher.rabotin@gmail.com>

ChristopherRabotin merged commit 7d6708c into nyx-space:master Nov 17, 2022

gwbres deleted the serdes branch September 2, 2023 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

epoch, duration: improve and fix serdes feature #175

epoch, duration: improve and fix serdes feature #175

gwbres commented Nov 15, 2022

gwbres commented Nov 15, 2022

ChristopherRabotin left a comment

ChristopherRabotin Nov 15, 2022

ChristopherRabotin Nov 15, 2022

gwbres Nov 15, 2022

ChristopherRabotin Nov 15, 2022

gwbres Nov 15, 2022 •

edited

Loading

gwbres Nov 15, 2022 •

edited

Loading

ChristopherRabotin Nov 15, 2022

gwbres Nov 15, 2022 •

edited

Loading

ChristopherRabotin Nov 15, 2022

gwbres Nov 15, 2022 •

edited

Loading

codecov-commenter commented Nov 15, 2022 •

edited

Loading

ChristopherRabotin Nov 15, 2022

gwbres Nov 15, 2022

ChristopherRabotin Nov 15, 2022

ChristopherRabotin commented Nov 16, 2022

	#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
	#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]

epoch, duration: improve and fix serdes feature #175

epoch, duration: improve and fix serdes feature #175

Conversation

gwbres commented Nov 15, 2022

gwbres commented Nov 15, 2022

ChristopherRabotin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwbres Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

gwbres Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwbres Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwbres Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Nov 15, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopherRabotin commented Nov 16, 2022

gwbres Nov 15, 2022 •

edited

Loading

gwbres Nov 15, 2022 •

edited

Loading

gwbres Nov 15, 2022 •

edited

Loading

gwbres Nov 15, 2022 •

edited

Loading

codecov-commenter commented Nov 15, 2022 •

edited

Loading