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

switch from time to systemtime #53

Merged
merged 2 commits into from
Jul 30, 2022
Merged

Conversation

esheppa
Copy link
Contributor

@esheppa esheppa commented Jul 23, 2022

This reduces a dependency, but also makes ulid compatible with all datetime libraries as the internal types generally implement From<SystemTime>.

This should also solve #52 and potentially #50

@dylanhart dylanhart self-assigned this Jul 26, 2022
@dylanhart dylanhart self-requested a review July 27, 2022 03:48
@dylanhart dylanhart removed their assignment Jul 27, 2022
Copy link
Owner

@dylanhart dylanhart left a comment

Choose a reason for hiding this comment

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

Hi Eric! Thank you for you contribution. Currently the PR is not compiling:

$ cargo test
   Compiling ulid v0.6.0
error[E0433]: failed to resolve: use of undeclared type `Duration`
   --> src\generator.rs:182:46
    |
182 |         let ulid3 = Ulid::from_datetime(dt + Duration::from_millis(1));
    |                                              ^^^^^^^^ not found in this sc
ope
    |
help: consider importing one of these items
    |
174 |     use core::time::Duration;
    |
174 |     use std::time::Duration;
    |

For more information about this error, try `rustc --explain E0433`.
error: could not compile `ulid` due to previous error
warning: build failed, waiting for other jobs to finish...

Can you fix the issues? The following should all pass:

cargo test
cargo test --all-features
cargo test --no-default-features --features=std
cargo test --no-default-features

Thanks!

@esheppa
Copy link
Contributor Author

esheppa commented Jul 27, 2022

Thanks for the review @dylanhart - I've fixed the compile error and verified with the cargo test commands you listed. I also fixed the cli which I'd broken due to SystemTime not implementing Display, so I've added that to the CI checks as well

Copy link
Owner

@dylanhart dylanhart left a comment

Choose a reason for hiding this comment

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

Hi Eric! Thanks for the update. Can you remove the panicking .unwrap() calls? Thanks!

src/generator.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/time.rs Outdated Show resolved Hide resolved
fix compile failure, clippy lints, check of cli in ci

handle SystemTimes before UNIX_EPOCH by truncating

add empty feature to avoid breaking users with the feature selected
@esheppa
Copy link
Contributor Author

esheppa commented Jul 29, 2022

Thanks @dylanhart - I've fixed those unwrap calls - let me know if you are ok with the additional wording in the docs

Copy link
Owner

@dylanhart dylanhart left a comment

Choose a reason for hiding this comment

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

This looks great. The code and rustdoc look good to me. Just one comment on cargo features.

Cargo.toml Outdated Show resolved Hide resolved
@esheppa
Copy link
Contributor Author

esheppa commented Jul 30, 2022

I've cleaned up the Cargo.toml and README.md. I've left the benchmarks as is as I assume you'd like to run them on your computer for consistency

@dylanhart dylanhart merged commit 1e30316 into dylanhart:master Jul 30, 2022
@dylanhart
Copy link
Owner

Looks good! Thanks! Will make a release for this later.

@esheppa esheppa deleted the use-systemtime branch July 31, 2022 03:54
@dylanhart
Copy link
Owner

Available in v1.0.0

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.

2 participants