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

Change chrono dep to time #116

Merged
merged 2 commits into from
Dec 19, 2022
Merged

Change chrono dep to time #116

merged 2 commits into from
Dec 19, 2022

Conversation

TimonPost
Copy link
Member

@TimonPost TimonPost commented Dec 19, 2022

Reason:

  • Chrono depends on very old time 1.43 dep that has security issues.
  • By having chrono we require to have two version of time 1.43 and 0.3.9
  • Chrono Includes quite a few deps that can be ignored if one only needs to depend on egui puffin.
  • Time is lower level and we only use it for date parsing which can easily be done by time crate only

@TimonPost TimonPost changed the title Change chrono to time Change chrono dep to time Dec 19, 2022
Copy link
Collaborator

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Good call getting rid of chrono

puffin_egui/Cargo.toml Show resolved Hide resolved
puffin_egui/src/lib.rs Show resolved Hide resolved
puffin_egui/src/lib.rs Show resolved Hide resolved
@@ -104,6 +104,7 @@ use std::{
fmt::Write as _,
sync::{Arc, Mutex},
};
use time::{macros::format_description, OffsetDateTime};
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can move the use statement to unside fn format_time

  • less pollution of file namespace
  • when reading format_time it's easier to find where the names come from

@mergify mergify bot merged commit 8b750a5 into main Dec 19, 2022
@mergify mergify bot deleted the timon/change-chrono-to-time branch December 19, 2022 13:39
@TimonPost
Copy link
Member Author

OH, bot merged it. Will do the changes!

@emilk
Copy link
Collaborator

emilk commented Dec 19, 2022

the bot merged a red PR? 🤦

@@ -21,7 +21,7 @@ include = [
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
chrono = "0.4"
time = { version = "0.3.17", default-features = false, features = [ "parsing" ] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

you are not really parsing time though?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, it, changed to formatting, and also had to enable the macro feature flag. I tested my changes from root repo, turns out, when I compiled puffin egui only, it had compile errors. This was because other dependencies included the macro feature flag which puffin egui does not depend on. I directly pushed to main to fix those issues.

Also released a new patch version for your flamegraph and this change.

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

2 participants