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
Allow two-digit-max-year customization in patterns #1721
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1721 +/- ##
=======================================
Coverage 99.35% 99.35%
=======================================
Files 155 155
Lines 6988 7024 +36
=======================================
+ Hits 6943 6979 +36
Misses 45 45
☔ View full report in Codecov by Sentry. |
2fe5b16
to
6cf5ce2
Compare
It's been a while, but I think this is ready now :) Lots of copy/paste, so if you find any typos in docs etc, I'll no doubt need to fix them everywhere... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/// <summary> | ||
/// Maximum two-digit-year in the template to treat as the current century. | ||
/// </summary> | ||
public int TwoDigitYearMax { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: worth adding a <value>
per above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/NodaTime/Text/InstantPattern.cs
Outdated
/// <summary> | ||
/// Creates a pattern like this one, but with a different <see cref="TwoDigitYearMax"/> value. | ||
/// </summary> | ||
/// <param name="twoDigitYearMax">The value to use for <see cref="TwoDigitYearMax"/> in the new pattern.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth adding something like "The value (0-99) to use..." to make the range clear?
Separately, where do we document the default values for this type? Probably we should document the default value for this new property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Will add the default into the description of the property.
Thanks for the review - will merge when I've finished tweaking the docs, and when it's gone greeen. |
Fixes nodatime#1720, at least the main feature. Additional potential features to be implemented later: - WithTwoDigitYearMaxFromCalendar - An AppContext switch to prohibit two-digit years in patterns
No description provided.