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 support for chrono::DateTime<T> #114

Merged
merged 3 commits into from
Jul 23, 2024

Conversation

emwalker
Copy link
Contributor

@emwalker emwalker commented Jul 20, 2024

Draft PR to add support for converting to and from chrono::DateTime<Utc> and chrono::DateTime<FixedOffset> timestamps. Related discussion. I'm out of my depth here both with respect to the Ruby C API and with respect to usage of chrono, so additional diligence might be needed.

I am not attached to this PR at all. With support for nanosecond precision recently added (still to be tested on our side), it is possible that we will not need chrono.

Copy link
Owner

@matsadler matsadler 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 good, I'd be happy to merge this as it currently is.

I'd add an entry in the changelog, and a mention here in the readme:

| `std::time::SystemTime`, `magnus::Time` | `Time` |

similar to how bytes get a footnote calling out that it requires a feature:

| `String`, `PathBuf`, `char`, `magnus::RString`, `bytes::Bytes`\*\*\* | `String`, `#to_str` |

\*\*\* when the `bytes` feature is enabled

But I'm happy to make those changes myself after merging.

src/time.rs Show resolved Hide resolved
@emwalker
Copy link
Contributor Author

similar to how bytes get a footnote calling out that it requires a feature:

Given how the table is already a pretty cramped, it seemed like four **** might be a bit much. I took the liberty of converting the footnotes to GitHub Markdown's footnotes. Let me know if you would like me to revert to the four asterisks.

2024-07-22_16-45-footnotes

Unfortunately, the footnotes appear at the bottom of the page instead of the table:

2024-07-22_16-46-footnotes

@emwalker emwalker requested a review from matsadler July 22, 2024 22:50
@emwalker emwalker marked this pull request as ready for review July 22, 2024 22:52
@matsadler
Copy link
Owner

it seemed like four **** might be a bit much. I took the liberty of converting the footnotes to GitHub Markdown's footnotes.
...
Unfortunately, the footnotes appear at the bottom of the page...

You're right, **** is a bit much. Probably the *** that was already there was a bit much.

I didn't know about markdown footnotes, that's neat. The readme is also rendered on crates.io, but I checked and that supports footnotes too.

However I think the way it adds them to the bottom of the page is a dealbreaker for me, especially with such a long page.

Maybe we could use *, †, ‡, §, ||, and ¶ instead of ever more asterisks?

@emwalker
Copy link
Contributor Author

emwalker commented Jul 23, 2024

Updated.

In typesetting, I think the symbols would be used in order of *, †, ‡, §, ||, and ¶ as footnote references are encountered in the table, and then the footnotes themselves would be sorted in that order, in contrast to what I just did. The symbols would also be superscripts, both in the references and in the footnotes. But I didn't want to get fancy. I'm sure you'll update it to something nice.

@matsadler
Copy link
Owner

This is great, thanks!

@matsadler matsadler merged commit 4125ee2 into matsadler:main Jul 23, 2024
43 checks passed
@emwalker emwalker deleted the chrono-timestamps branch July 23, 2024 13:19
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