-
Notifications
You must be signed in to change notification settings - Fork 657
Support C# format strings in config #4605
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
base: main
Are you sure you want to change the base?
Conversation
f192c56
to
92fa63f
Compare
please rebase and resolve the conflicts |
92fa63f
to
9a18634
Compare
e7652ec
to
16f557c
Compare
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.
Awesome work! ❤️
3e5bf0e
to
65a39f5
Compare
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.
Besides the build failure, this is looking really good! The only missing part is documentation.
c7fbede
to
646a634
Compare
53547ee
to
625c32f
Compare
@9swampy if you don't mind I'll have a review of this in 2 weeks time as I'm on vacation. I need to understand it a bit in detail the changes as there is a chance we will need to maintain these changes. |
Can I also suggest adding some tests that use the zero-suppression syntax so that we can get versions without a commits number e.g.
|
0c3e0c0
to
f0e2983
Compare
@9swampy could you please rebase your branch onto latest |
@9swampy I had a look on the code, looks amazing. There are a couple of things that I think we can do to make it merge-able: |
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.
Move IInputSanitizer
and the InputSanitizer
as well as StringFormatWithExtension
to Formatting
folder
c125c9a
to
77a4d38
Compare
Done, with Tests. Took liberty of renaming StringFormatWith filename to match contents. Aligns with other extension[s] filenames. Saw you'd an extra omit under with rebase done. To fix the "conflict" I presume? Sorry I wasn't sure what was going on there, neither option worked so I just left it. Green now - not - docs blowing up? |
Not familiar with the codebase but this is rumbling up quite a ripple effect; dependency inversion, new()s in the extension. Sure you want me to continue? |
I see there is also another usage within the |
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.
🤏🏼 so close now! Only a few more nits. Great work! ❤️
## Verified Examples | ||
|
||
The following examples are verified by actual unit tests in the GitVersion codebase: | ||
|
||
### Zero-Padded Numeric Formatting | ||
|
||
```yaml | ||
assembly-informational-format: "{Major}.{Minor}.{Patch}-{CommitsSinceVersionSource:0000}" | ||
``` | ||
|
||
**Test**: `VariableProviderTests.Format_Allows_CSharp_FormatStrings()` | ||
**Input**: `CommitsSinceVersionSource = 42` | ||
**Output**: `"1.2.3-0042"` | ||
|
||
### String Case Transformations | ||
|
||
```csharp | ||
// From StringFormatterTests.cs | ||
[TestCase("hello world", "c", "HelloWorld")] // PascalCase | ||
[TestCase("hello", "u", "HELLO")] // Uppercase | ||
[TestCase("HELLO", "l", "hello")] // Lowercase | ||
[TestCase("hello world", "t", "Hello World")] // Title Case | ||
[TestCase("hELLO", "s", "Hello")] // Sentence Case | ||
``` | ||
|
||
### Numeric Format Specifiers | ||
|
||
```csharp | ||
// From NumericFormatterTests.cs | ||
[TestCase("1234.5678", "n", "1,234.57")] // Number format | ||
[TestCase("1234.5678", "f2", "1234.57")] // Fixed-point format | ||
[TestCase("1234.5678", "f0", "1235")] // No decimals | ||
``` | ||
|
||
### Date Formatting | ||
|
||
```csharp | ||
// From DateFormatterTests.cs | ||
[TestCase("2021-01-01", "yyyy-MM-dd", "2021-01-01")] | ||
[TestCase("2021-01-01T12:00:00Z", "yyyy-MM-ddTHH:mm:ssZ", "2021-01-01T12:00:00Z")] | ||
``` | ||
|
||
### Currency and Percentage (InvariantCulture) | ||
|
||
```csharp | ||
// From FormattableFormatterTests.cs | ||
[TestCase(123.456, "C", "¤123.46")] // Generic currency symbol | ||
[TestCase(123.456, "P", "12,345.60 %")] // Percentage format | ||
[TestCase(1234567890, "N0", "1,234,567,890")] // Thousands separators | ||
``` |
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.
I don't think we need to advertise code coverage in our documentation. If the test cases contain essential information, we should provide them in a more human-friendly format.
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.
@9swampy I left only this change on you, all the other requested changes were addressed
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.
I don't think we need to advertise code coverage in our documentation. If the test cases contain essential information, we should provide them in a more human-friendly format.
If that's the worst criticism you had on "my" AI generated ramblings, that's a win. Did you check the links are good?
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.
@9swampy please fix the docs and then I guess we can merge
src/GitVersion.Core.Tests/Formatting/FormattableFormatterTests.cs
Outdated
Show resolved
Hide resolved
src/GitVersion.Core.Tests/Formatting/SanitizeEnvVarNameTests.cs
Outdated
Show resolved
Hide resolved
src/GitVersion.Core.Tests/Formatting/SanitizeMemberNameTests.cs
Outdated
Show resolved
Hide resolved
67d7562
to
d68e4a6
Compare
Renames `PascalCase` to `ToPascalCase` and moves `TextInfo` parameter to improve clarity and usability. Refactors related calls to align with the updated method signature.
b69d323
to
cc37e2b
Compare
Dang. Sry, missed those. I'm debateably OCD to the point editorconfig's become a crutch I depend on, clearly fall over without. |
@9swampy will you have time to finalize this one? We want to have a new release and would be nice to include this PR as well |
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.
Having now spun up the documentation and read through it, there's one thing I don't quite understand. In what looks like code examples, we find stuff like this:
# GitVersion.yml
template: "{Major}.{Minor}.{Patch:F2}-{PreReleaseLabel}"
This gives the impression that template
is a supported configuration key in GitVersion.yml
. Afaik, it is not? Fixing the examples so they only provide code that actually works in GitVersion.yml
and removing the test cases and I think we're good!
No worries, it was AI generated. The way I read it was:
It didn't mean to imply there was a configuration key called template, but I see where you're coming from. This and the test verified examples: gone. Really wasn't comfortable with the docs, that's why I marked them "draft". would be nice to make it in to the imminent release so if there's more tweaks don't be shy. |
@asbjornu if you find all these changes good, please merge this PR |
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: