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

epoch, duration: improve and fix serdes feature #175

Merged
merged 8 commits into from
Nov 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ jobs:

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

- name: Test (serde)
run: cargo test --features serde

test_no_std:
strategy:
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ num-traits = {version = "0.2.15", default-features = false, features = ["libm"]}
lexical-core = {version = "0.8.5", default-features = false, features = ["parse-integers", "parse-floats"]}

[dev-dependencies]
serde_json = "1"
criterion = "0.4.0"

[features]
Expand Down
37 changes: 13 additions & 24 deletions src/duration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

@gwbres gwbres Nov 15, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@gwbres gwbres Nov 15, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@gwbres gwbres Nov 15, 2022

Choose a reason for hiding this comment

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

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

use serde_derive::{Deserialize, Serialize};

use core::str::FromStr;

Expand Down Expand Up @@ -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))]
Copy link
Member

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", 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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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"

pub struct Duration {
pub(crate) centuries: i16,
pub(crate) nanoseconds: u64,
Expand Down Expand Up @@ -758,18 +759,6 @@ impl Duration {
}
}

#[cfg(feature = "std")]
#[cfg(not(kani))]
impl<'de> Deserialize<'de> for Duration {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let s = String::deserialize(deserializer)?;
FromStr::from_str(&s).map_err(de::Error::custom)
}
}

impl Mul<i64> for Duration {
type Output = Duration;
fn mul(self, q: i64) -> Self::Output {
Expand Down Expand Up @@ -1201,20 +1190,20 @@ impl From<std::time::Duration> for Duration {
}
}

#[cfg(feature = "std")]
#[test]
fn deser_test() {
use serde_derive::Deserialize;
#[derive(Deserialize)]
struct _D {
pub _d: Duration,
}
}

#[cfg(kani)]
#[kani::proof]
fn formal_normalize_any() {
let centuries: i16 = kani::any();
let nanoseconds: u64 = kani::any();
let _dur = Duration::from_parts(centuries, nanoseconds);
}

#[test]
#[cfg(feature = "serde")]
fn test_serdes() {
let dt = Duration::from_seconds(10.1);
let content = r#"{"centuries":0,"nanoseconds":10100000000}"#;
assert_eq!(content, serde_json::to_string(&dt).unwrap());
let parsed: Duration = serde_json::from_str(content).unwrap();
assert_eq!(dt, parsed);
}
39 changes: 13 additions & 26 deletions src/epoch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg(feature = "serde")]
#[cfg(feature = "std")]

Copy link
Collaborator Author

@gwbres gwbres Nov 15, 2022

Choose a reason for hiding this comment

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

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

use serde_derive::{Deserialize, Serialize};

use core::str::FromStr;
#[cfg(feature = "std")]
Expand Down Expand Up @@ -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))]
Copy link
Member

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", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]

pub struct Epoch {
/// An Epoch is always stored as the duration of since J1900 in the TAI time scale.
pub duration_since_j1900_tai: Duration,
Expand Down Expand Up @@ -2477,18 +2478,6 @@ impl FromStr for Epoch {
}
}

#[cfg(feature = "std")]
#[cfg(not(kani))]
impl<'de> Deserialize<'de> for Epoch {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let s = String::deserialize(deserializer)?;
FromStr::from_str(&s).map_err(de::Error::custom)
}
}

impl fmt::Debug for Epoch {
/// Print this epoch in Gregorian in the time scale used at initialization
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
Expand Down Expand Up @@ -2743,22 +2732,20 @@ fn leap_year() {
}
}

#[cfg(feature = "std")]
#[test]
fn deser_test() {
use serde_derive::Deserialize;
#[derive(Deserialize)]
struct _D {
pub _e: Epoch,
}

println!("{}", (1 * Unit::Century + 12 * Unit::Hour).to_seconds());
}

#[test]
fn cumulative_days_for_month() {
assert_eq!(
CUMULATIVE_DAYS_FOR_MONTH,
[0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334]
)
}

#[test]
#[cfg(feature = "serde")]
fn test_serdes() {
let e = Epoch::from_gregorian_utc(2020, 01, 01, 0, 0, 0, 0);
let content = r#"{"duration_since_j1900_tai":{"centuries":1,"nanoseconds":631065637000000000},"time_scale":"UTC"}"#;
assert_eq!(content, serde_json::to_string(&e).unwrap());
let parsed: Epoch = serde_json::from_str(content).unwrap();
assert_eq!(e, parsed);
}
10 changes: 10 additions & 0 deletions src/timescale.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,3 +186,13 @@ impl FromStr for TimeScale {
}
}
}

#[test]
#[cfg(feature = "serde")]
fn test_serdes() {
let ts = TimeScale::UTC;
let content = "\"UTC\"";
assert_eq!(content, serde_json::to_string(&ts).unwrap());
let parsed: TimeScale = serde_json::from_str(content).unwrap();
assert_eq!(ts, parsed);
}