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

defmt impls for time #763

Open
burrbull opened this issue Jun 27, 2023 · 6 comments
Open

defmt impls for time #763

burrbull opened this issue Jun 27, 2023 · 6 comments
Labels
difficulty: easy Pretty easy to solve good first issue Good for newcomers type: enhancement Enhancement or feature request

Comments

@burrbull
Copy link

It makes sense to have implementations for time crate primitives.

@jonathanpallant
Copy link
Contributor

Maybe, but there's an almost unlimited number of third-party types we could special case.

Also, most embedded projects seem to use https://docs.rs/chrono/0.4.26/chrono/ and not time.

@Urhengulas
Copy link
Member

I agree that there is a potentially unbound number. But I also think that either us or them should implement it, so users can use these types easily with defmt. And since both time and chrono are bigger than defmt I guess it makes sense to do it on our side.

Long story short, I'd be okay with accepting a PR to add Format implements for the time crate, hidden behind a feature gate. Same for chrono.

@Urhengulas Urhengulas added type: enhancement Enhancement or feature request good first issue Good for newcomers difficulty: easy Pretty easy to solve labels Jul 26, 2023
@Urhengulas
Copy link
Member

There is chronotope/chrono#1632 to implement defmt::Format for the chrono structs.

@burrbull
Copy link
Author

burrbull commented Jan 8, 2025

I still think this should be done on defmt side. chrono and time are common used crates implementing basic time types.
Where defmt is "embedded only" logging framework.

@jonathanpallant
Copy link
Contributor

Yeah, but if chrono or time make breaking change bumps, we have to implement the traits for all common versions, which gets messy.

We're just about to commit to never doing a major version bump, so it's cleaner for them to implement our trait. And DJC said he would once we hit 1.0.

@Urhengulas
Copy link
Member

Also if we implement it on the chrono or time side we have access to private fields. I don't know if this is necessary in this case but it makes sense in the general case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy Pretty easy to solve good first issue Good for newcomers type: enhancement Enhancement or feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants