Add feature gated defmt support.

[pebender]

This pull request adds defmt support gated behind Cargo.toml feature. It builds on the work of others and updates to the current chrono repo.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.90%. Comparing base (334565c) to head (06b6be7).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1747   +/-   ##
=======================================
  Coverage   90.90%   90.90%           
=======================================
  Files          39       39           
  Lines       16262    16262           
=======================================
  Hits        14783    14783           
  Misses       1479     1479           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[djc]

LGTM. Please squash all of these commits into a single one.

[pebender]

I have squashed them into a single commit.

[pebender]

BTW, I have pull requests to add defmt to chrono-tz and pure-rust-locales. Once the pure-rust-locales had defmt support, there is one defmt addition to chrono that depends on pure-rust-locales having defmt support.

[djc]

BTW, I have pull requests to add defmt to chrono-tz and pure-rust-locales. Once the pure-rust-locales had defmt support, there is one defmt addition to chrono that depends on pure-rust-locales having defmt support.

Okay, let's merge this after the pure-rust-locales PR is done/released.

[pebender]

I have made the updates to reflect the 0.8.2 release of pure-rust-locales.

[djc]

Are these internals actually appearing anywhere? Not sure I want this stuff.

[pebender]

I tend to add defmt::Format to anything that implements Debug. However, as far as I can tell, the only reason that YearFlags and Mdf implement Debug is for the panic and assert messages found in test functions. So, I think removing defmt support YearFlags and Mdf would be fine. I will update the pull request.

[djc]

Okay, I dislike the maximalist take of doing all the Debug impls. I think you should do the ones that you personally have a need for and then we can add more as needed.

[djc]

I don't think we need these.

[pebender]

I have tried that route, and I have found it doesn't work well.

I am using mipidsi in my current project. I was improving error propagation, which required defmt on certain error message. So, I submitted a pull request that add defmt the this specific types. Then I added more error propagation and found I needed more error message to have defmt. Then I needed non error types because I was adding improved debug!, info!, warn!, and error! messages.

I am using headless in my current project. They failed to add defmt::Format support to one of their error types (for which the had implement Debug and Display). It turns out, I need defmt support on the error type. This has meant instead of using the released version, I have to use a git version. This would have been avoided had they added defmt::Format to every type they add Debug.

I encountered the same thing with chrono, chrono-tz and pure-rust-locales. I would need to add a chrono type to and error type or a diagnostic message, so I would defmt support. It would sometimes turn out that the churn type for which I needed defmt::support need additional defmt support in chrono-tz and pure-rust-locales. The I need another chrono type to support defmt, I would end up needing to add additional defmt support in chorine-tz.

I realized that if I followed the route of asking for defmt support as I discovered I needed it, there would be constant defmt pull requests and I would likely for to use my forks of projects because they would be the only ones that had the latest defmt I needed.

The, I relieved (1) Debug and defmt::Format are used for the same, and (2) at some point someone found a need for a specific type to need Debug support. Since Debug and defmt are used for the same thing and since Debug was needed for a specific type, it is likely that defmt will be needed for that specific type at some point. Therefore,adding defmt support seemed the most logical solution to the problem of not being able to use released crates, and not even being able to use crates's repo because defmt had been added by a process of add it as I discover I need it.

Like Debug, defmt::Format is used for every type of message from trace! to info!, and println!,and panic!.

[pebender]

Why do you think it is not needed? It is a public struct that needed Debug support at some point.

[djc]

I think you're vastly overestimating how careful people are about adding Debug implementations. I understand your point that iterating on which implementations are necessary is annoying, but I disagree that any Debug derive or impl warrants a corresponding Format impl. I am maintaining chrono on a volunteer basis and you keep pushing back on my review feedback, causing multiple round trips that should have been unnecessary. I don't want to do a few more rounds of this.

I'm open to having impls for core public api types like DateTime, NaiveDate, etc.

I also understand the point about having them for error types.

I definitely don't want them for private types, for test-only types, or for tightly scoped types like these serde visitor impls, that are very unlikely to be reachable from the public API anyway.

I'll try one more review after this to see if we're close to something I can merge, please stop arguing now.

[pebender]

As you can see from the comments and the code, I have taken your review feedback. I removed all the defmts that were added to non-public facing APIs. In addition, I removed a couple others that I believed where unlikely to be used even though they were public (e.g. iterators)

However, when the review feedback says "I don't like this" (and nothing else) and "add them one-by-one rather than all at once to the public interface" with no technical or operational reason and I am going to explain my reasons.

I don't understand why you consider this not taking feedback.

By the way, the chrono project has very few clippy directives in src/lib.rs. However, one of them is missing_debug_implementations. So, at some point, the attitude of the chrono project developers was to add Debug to everything not to add it as needed. I suspect their reason is the same reason as mine.

I appreciate that your work is a voluntary. And I appreciate that you take the time to do it.

My work is voluntary as well. Taking the time to submit pull requests and iterate over them based on the feedback of reviewers is voluntary. It is easier for me to just fork the project and (when necessary) release a version that meets my needs and the needs of others who would like defmt support or the same bug fixed. It is time consuming to split and maintain each topic into a separate branch and pull request while ensuring that the version I am using stays in sync with all the separate pull requests. This is especially time consuming when there are interdependent projects (like chrono, chono-tz and and pure-rust-locales). This is especially time consuming when the projects don't past its own CI because tools have changed or libraries on which they depend have changed as this forces a round of pull requests requests or bug reports to get CI working so that other pull requests will pass CI (this is is almost every project). However, I choose to take the time because it's more likely to help more people to have it in the mainline repository and release.

[djc]

As you can see from the comments and the code, I have taken your review feedback. I removed all the defmts that were added to non-public facing APIs. In addition, I removed a couple others that I believed where unlikely to be used even though they were public (e.g. iterators)

Thanks for that. I've merged this now.

I don't understand why you consider this not taking feedback.

I just got frustrated by you restating your reasons with large walls of text.

By the way, the chrono project has very few clippy directives in src/lib.rs. However, one of them is missing_debug_implementations. So, at some point, the attitude of the chrono project developers was to add Debug to everything not to add it as needed. I suspect their reason is the same reason as mine.

Good point, dropping that in #1753.

However, I choose to take the time because it's more likely to help more people to have it in the mainline repository and release.

Appreciated, though I'll note that (compared to forking) this also just helps relieve yourself from future maintenance of the fork, whereas by contributing upstream you're asking a maintainer to indefinitely maintain additional surface. So I still think it's fairly asymmetrical.

[pebender]

Thank you for taking the time to review this pull request and for merging it.

I can get rather verbose. Sorry about that.

Yes, contributing it upstream means that that the maintenance workload shifts. I try to may pull requests either bug fixes for features other people appear to want (e.g. forked the repo to develop the feature as well).

By the way, I suspect you can close pull requests #1632 and #1655