Support C# format strings in config #4605
Conversation
9swampy
force-pushed
the
SupportFormatStringsInConfig
branch
7 times, most recently
from
f192c56 to
92fa63f
Compare
|
please rebase and resolve the conflicts |
9swampy
force-pushed
the
SupportFormatStringsInConfig
branch
from
92fa63f to
9a18634
Compare
9swampy
force-pushed
the
SupportFormatStringsInConfig
branch
from
9a18634 to
e7652ec
Compare
9swampy
force-pushed
the
SupportFormatStringsInConfig
branch
from
e7652ec to
16f557c
Compare
| if (value is string dateStr && DateTime.TryParse(dateStr, out var parsedDate) && format.StartsWith("date:")) | ||
| { | ||
| var dateFormat = format.Substring(5); | ||
| result = parsedDate.ToString(dateFormat, CultureInfo.InvariantCulture); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Couldn't we just always TryParse strings to DateTime? If it's successful, it's a date that can be formatted directly by the format string, removing the need for the date: prefix, no?
There was a problem hiding this comment.
Adding a "date:" prefix to the format string has design advantages rather than technical necessity. It allows for format disambiguation, extensibility, and robustness in multi-formatter scenarios.
If you have multiple formatters (e.g., DateFormatter, NumericFormatter), they may all receive the same input format string (like "yyyy-MM-dd"). Without the "date:" prefix, each formatter has to guess whether the format is relevant.
With and the formatter can early-return if the format doesn’t match their expected prefix.
Absolutely, if you prefer without confirm and I'll make it so.
There was a problem hiding this comment.
I see the dilemma. It would be beneficial to examine the scenarios that may lead to ambiguity and explore how we can best mitigate them.
| format.Equals("C", StringComparison.OrdinalIgnoreCase) || | ||
| format.Equals("P", StringComparison.OrdinalIgnoreCase) || | ||
| format.StartsWith("N", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Why are these format strings blocked?
There was a problem hiding this comment.
I'd thought they'd create formatted strings with undesirable characters; comma, $, £, %. Opinion; happy to take a steer.
There was a problem hiding this comment.
Why would they be undesirable?
| @@ -0,0 +1,31 @@ | |||
| using System.Globalization; | |||
|
|
|||
| namespace GitVersion.Helpers; | |||
There was a problem hiding this comment.
I'm not a huge fan of the Helpers namespace. Can we please put all of this into a more descriptive GitVersion.Formatting (or similar) namespace instead?
There was a problem hiding this comment.
I've an aversion to Helper classes but that's where I've dropped all this as an extension of the original StringFormatWith but please redirect if it should live elsewhere (or just needs some structure).
I'll move to GitVersion.Formatting as suggested.
There was a problem hiding this comment.
Moved formatters and their tests. Let me know if more should move; those left are coupled more closely with the extension entrypoint.
| using System.Text.RegularExpressions; | ||
| using GitVersion.Core; | ||
| using GitVersion.Formatting; |
There was a problem hiding this comment.
The type or namespace name 'Formatting' does not exist in the namespace 'GitVersion' (are you missing an assembly reference?)
Unclear what's gone wrong here. Clearly that's what the PR introduces and VS is happy. Is there something in the pipeline to prevent unapproved namespace additions?
There was a problem hiding this comment.
Weird. Not that I know of. @arturcic, do you have an idea?
Description
Support C# format strings in config
Related Issue
Fixes #4156
Motivation and Context
#4156
How Has This Been Tested?
I've implemented a number of formatters and added scenario coverage for implementation but a 4-eyes verification and feedback on appropriateness of scenarios included (or not) would be welcome. The specific scenrio noted on the wish-issue tested here and here.
I've an aversion to Helper classes but that's where I've dropped all this as an extension of the original StringFormatWith but please redirect if it should live elsewhere (or just needs some structure).
Screenshots (if appropriate):
Checklist: