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 unix_timestamp_offset to TimeConfig #45

Merged
merged 9 commits into from
Jul 13, 2023

Conversation

adriangb
Copy link
Member

@adriangb adriangb commented Jul 11, 2023

Fix #41.

@adriangb
Copy link
Member Author

please review

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #45 (96d31a1) into main (ed75c5b) will decrease coverage by 0.11%.
The diff coverage is 93.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
- Coverage   99.17%   99.07%   -0.11%     
==========================================
  Files           6        6              
  Lines         847      862      +15     
==========================================
+ Hits          840      854      +14     
- Misses          7        8       +1     
Impacted Files Coverage Δ
src/lib.rs 91.66% <0.00%> (-8.34%) ⬇️
src/time.rs 97.51% <92.30%> (+0.07%) ⬆️
src/datetime.rs 100.00% <100.00%> (ø)
src/duration.rs 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed75c5b...96d31a1. Read the comment docs.

@samuelcolvin
Copy link
Member

I think this should be unix_tz a bool or enum with two options.

@adriangb
Copy link
Member Author

or enum with two options

It is an enum with two options

@dmontagu
Copy link

dmontagu commented Jul 11, 2023

I think this should be unix_tz a bool or enum with two options.

Do you mean the name of the field on TimeConfig? Or something else?

#[derive(Debug, Clone, Default, Copy)]
pub enum DefaultTimeOffset {
    #[default]
    Naive,
    Utc,
}

// ...

#[derive(Debug, Clone, Default)]
pub struct TimeConfig {
    pub microseconds_precision_overflow_behavior: MicrosecondsPrecisionOverflowBehavior,
    pub default_time_offset: DefaultTimeOffset,
}

Are you just suggesting changing default_time_offset to unix_tz?

@samuelcolvin
Copy link
Member

If the input is a number or a numeric string both of which are interpreted as Unix timestamps, then this setting should decide if the timezone is None or Some(0).

Hence the struct member name should change to unix_tz.

@adriangb
Copy link
Member Author

src/time.rs Outdated
@@ -320,6 +349,11 @@ impl Time {
}
}

let tz_offset = match (tz_offset, config.default_time_offset) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be removed.

@adriangb
Copy link
Member Author

please review

src/datetime.rs Show resolved Hide resolved
src/time.rs Show resolved Hide resolved
src/duration.rs Show resolved Hide resolved
src/time.rs Outdated Show resolved Hide resolved
src/time.rs Show resolved Hide resolved
src/time.rs Show resolved Hide resolved
Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Seems good to me but best to wait for @samuelcolvin to confirm as they have a clear idea of what needs to happen.

@samuelcolvin samuelcolvin merged commit 7058870 into main Jul 13, 2023
@samuelcolvin samuelcolvin deleted the add-config-default-timezone branch July 13, 2023 11:28
@samuelcolvin samuelcolvin changed the title Add default_time_offset to TimeConfig Add unix_timestamp_offset to TimeConfig Jul 13, 2023
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.

Allow setting Unix timestamps timezone to UTC+0
4 participants