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 to millisecond resolution (fixes #2) #3

Merged
merged 2 commits into from
Jun 2, 2024

Conversation

eddelbuettel
Copy link
Owner

This uses the fork by Chris Bove which relies on std::chrono objects instead of time_t which permits the complete flowthrough of millisecond resolution. The R wrapper was updated accordingly and an updated demo is now in the README.md

@dmurdoch
Copy link

dmurdoch commented May 31, 2024

Runs fine here. Comments:

  • The ChangeLog from yesterday has a typo: READNE.md instead of README.md.
  • There are lots of whitespace changes in the new src/ulid/* files. Those probably came from the fork you're using, but it might be worth checking just to avoid introducing unnecessary changes.
  • You already mentioned the C++17 dependency and need for help updates.

@eddelbuettel
Copy link
Owner Author

eddelbuettel commented May 31, 2024

Thanks for the feedback. As to your bullet points, in order

  • will fix the typo (done now in my sources)
  • such is life; I copied the files from the aforementioned fork, I won;t hand edit them
  • yup (I may just cheat and impose R (>= 4.3.0) which will default to C++17 (adding src/Makevars* oneliners instead)

What platforms did you build? macOS?

@dmurdoch
Copy link

Platform was macOS 14.5 (Sonoma), with an M3 chip.

@eddelbuettel
Copy link
Owner Author

@markheckmann you may want to give this a whirl, as the (updated) README.md shows we now cover milli-seconds based on the fork switching that std::chrono (that I was unaware of ...)

> library(ulid)
> gen_ulid <- \(sleep) replicate(5, {Sys.sleep(sleep); generate()})
> u <- gen_ulid(.1)
> df <- unmarshal(u)
> data.table::data.table(df)
                        ts              rnd
                    <POSc>           <char>
1: 2024-05-30 16:38:28.588 CSQAJBPNX75R0G5A
2: 2024-05-30 16:38:28.688 XZX0TREDHD6PC1YR
3: 2024-05-30 16:38:28.789 0YK9GKZVTED27QMK
4: 2024-05-30 16:38:28.890 SC3M3G6KGPH7S50S
5: 2024-05-30 16:38:28.990 TSKCBWJ3TEKCPBY0
>

@eddelbuettel eddelbuettel merged commit cd08e0a into master Jun 2, 2024
2 checks passed
@eddelbuettel eddelbuettel deleted the feature/upstream_enhancements branch June 3, 2024 16:22
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