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

Mark format::Numeric and format::Fixed as non_exhaustive #1409

Closed
pitdicker opened this issue Feb 3, 2024 · 5 comments · Fixed by #1430
Closed

Mark format::Numeric and format::Fixed as non_exhaustive #1409

pitdicker opened this issue Feb 3, 2024 · 5 comments · Fixed by #1430
Labels
API-incompatible Tracking changes that need incompatible API revisions

Comments

@pitdicker
Copy link
Collaborator

One thing that makes improving our parsing and formatting code difficult is that adding new formatting items is a breaking change.

Almost all formatting items live in format::Numeric and format::Fixed enums within a format::Item.

User code matching on the these enums within a format::Item is very rare. When we did add variants to it in 0.4.23 it broke one abandoned crate (see #899, #1151).

format::Pad could also benefit from becoming non_exhaustive, but there is a slightly higher chance people are exhaustively matching on its three variants.

For 0.5 we should mark these enums as non_exhaustive. And given the low impact of the accidental previous change we may consider it on 0.4.x.

@pitdicker pitdicker added the API-incompatible Tracking changes that need incompatible API revisions label Feb 3, 2024
@djc
Copy link
Member

djc commented Feb 7, 2024

Definitely sounds good to me for 0.5. For 0.4, what's the compatibility calculus here?

@pitdicker
Copy link
Collaborator Author

For 0.4, what's the compatibility calculus here?

That's a new term for me...

Two arguments:

  • Why would users want to exhaustively match on format::Numeric or format::Fixed?

    • Pretty much only when they want to reuse for strftime parser with their own implementation of formatting or parsing.
    • If they want to check if a strftime string includes / does not include some fields they might match on these items, or they may do a regex on the input string.

    Both seem to be rare (and easily fixed, although that doesn't make breakage good).

  • We had a kind of crater check on this near the end of 2022 when adding variants to it broke one crate.

@djc
Copy link
Member

djc commented Feb 12, 2024

We had a kind of crater check on this near the end of 2022 when adding variants to it broke one crate.

Okay, and I assume it was a niche/obscure one? Sounds good to me, let's do it (and make sure to note it in the release notes).

@pitdicker
Copy link
Collaborator Author

Okay, and I assume it was a niche/obscure one?

It was chrono_locale, which has 13.252 all-time downloads, not seen an update for 5 years, and only one dependent crate (seemingly a hobby project).

@djc
Copy link
Member

djc commented Feb 12, 2024

Yes, that doesn't seem like a blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API-incompatible Tracking changes that need incompatible API revisions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants