-
Notifications
You must be signed in to change notification settings - Fork 662
Format no format #4657
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?
Format no format #4657
Conversation
7048ae1 to
5e1c165
Compare
8a331e5 to
3629aef
Compare
| (?<member>[A-Za-z_][A-Za-z0-9_]*) # member/property name | ||
| (?: # Optional format specifier | ||
| :(?<format>[A-Za-z0-9\.\-,]+) # Colon followed by format string (no spaces, ?, or }), format cannot contain colon | ||
| :(?<format>[A-Za-z0-9\.\-,;'"]+) # Colon followed by format string (including semicolons and quotes) |
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.
RCA
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.
RCA?
|
|
Apologies. Two passes is already more time than I had. The first pass was comfortable, the second's expanded in to assumptions that likely warrant a review & retrospect on regression pack coverage? Rollback & regroup the wise call? |
asbjornu
left a comment
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.
Fantastic work! 🎉 Thank you so much for jumping on this so quickly, @9swampy! ❤️
| # Three-section format: positive;negative;zero | ||
| assembly-informational-format: "{Value:positive;negative;zero}" | ||
| # Two-section format: positive;negative | ||
| assembly-informational-format: "{Value:pos;neg}" |
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.
Could these last two examples be modified to be more practical and real-world relevant, with a line showing the result, as in the first code example?
| - **Third section**: Used for zero values (optional) | ||
| - **Empty quotes** (`''` or `""`) create empty output | ||
|
|
||
| **Mixed Syntax:** You can combine legacy semicolon syntax with modern `??` fallback syntax in the same template. |
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.
How would ;; and ?? be combined? Can we give a code example for this 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.
I love the added tests! 🤩 Could we please have a test for the combination of ;; and ?? as well?
| using GitVersion.Formatting; | ||
|
|
||
| namespace GitVersion.Tests.Formatting; | ||
| namespace GitVersion.Core.Tests.Formatting; |
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.
GitVersion.Core should not be a namespace. :)
| namespace GitVersion.Core.Tests.Formatting; | |
| namespace GitVersion.Tests.Formatting; |
|
|
||
| // Currently this will throw or behave unexpectedly | ||
| // Should either throw meaningful error or handle gracefully | ||
| Assert.Throws<ArgumentException>(() => template.FormatWith(semanticVersion, environment)); |
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 think it's a good practice to test the returned ArgumentException to ensure that the right guard clause is throwing it. This just ensures that an ArgumentException is being thrown anywhere by the code path, not necessarily by our own code.
| using GitVersion.Formatting; | ||
|
|
||
| namespace GitVersion.Tests.Formatting; | ||
| namespace GitVersion.Core.Tests.Formatting; |
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.
| namespace GitVersion.Core.Tests.Formatting; | |
| namespace GitVersion.Tests.Formatting; |
| using GitVersion.Formatting; | ||
|
|
||
| namespace GitVersion.Tests.Formatting; | ||
| namespace GitVersion.Core.Tests.Formatting; |
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.
| namespace GitVersion.Core.Tests.Formatting; | |
| namespace GitVersion.Tests.Formatting; |
| (?<member>[A-Za-z_][A-Za-z0-9_]*) # member/property name | ||
| (?: # Optional format specifier | ||
| :(?<format>[A-Za-z0-9\.\-,]+) # Colon followed by format string (no spaces, ?, or }), format cannot contain colon | ||
| :(?<format>[A-Za-z0-9\.\-,;'"]+) # Colon followed by format string (including semicolons and quotes) |
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.
RCA?
|
@9swampy You mention "modern ?? fallback syntax" - what do you mean by that, AFAIK |
|
Honestly I'm embarrassed I've not got back to fixing this. It's been maniac at my day job and I've maxed the AI tooling out repeatedly so haven't had capacity to revisit. The reason to call that out now is to answer the question; don't get hung up on the modern quip. AIism. I will get a poke at the fix as soon as I can. |
|
@9swampy No worries - reason I was asking was to see if I could do the same idea of suppressing the commit number when it's zero via a different syntax |
|
@9swampy any updates on this one? |
|
Sorry, not yet. I will. |
0708d23 to
f6570b5
Compare
This commit fixes the bug where legacy .NET composite format syntax
like {CommitsSinceVersionSource:0000;;''} was not working correctly.
The issue was that the formatter would output literal text instead of
properly formatted values. For example, it would output:
"6.13.54-gv6-CommitsSinceVersionSource-0000-----"
instead of:
"6.13.54-gv60002"
Changes:
1. Updated RegexPatterns.cs to allow semicolons and quotes in format
strings by changing the format pattern from [A-Za-z0-9\.\-,]+ to
[A-Za-z0-9\.\-,;'"]+
2. Added LegacyCompositeFormatter.cs to properly handle the three-section
composite format syntax (positive;negative;zero) with support for:
- Zero-suppression using empty string ('')
- Proper handling of negative values (no double negative)
- Numeric formatting in all sections
3. Updated ValueFormatter.cs to register LegacyCompositeFormatter with
priority 1 and allow null values to be processed by formatters
4. Updated StringFormatWithExtension.cs to not return early for null
values when legacy syntax is detected
5. Added Issue4654Tests.cs to verify the fix works correctly
The fix ensures backward compatibility with legacy .NET composite format
syntax while maintaining support for modern format strings.
f6570b5 to
ac9485d
Compare
Adds all test files from the FormatNoFormat branch to ensure complete coverage of the LegacyCompositeFormatter functionality: - BackwardCompatibilityTests.cs (2 tests): Tests regex pattern parsing for legacy semicolon syntax and mixed new/old syntax - LegacyFormatterProblemTests.cs (9 tests): Comprehensive edge case tests covering: * Null value handling * Missing property error handling * Double negative prevention * Positive/negative/zero section selection * Numeric formatting in all sections * Invalid format handling * Issue GitTools#4654 verification tests - LegacyFormattingSyntaxTests.cs (7 tests): Tests for legacy format syntax parsing and behavior Also fixes namespace from GitVersion.Tests.Formatting to GitVersion.Core.Tests.Formatting in: - ValueFormatterTests.cs - StringFormatterTests.cs - DateFormatterTests.cs Total: 21 tests covering legacy composite format functionality
6560a97 to
b638fe2
Compare
|



Fix legacy .NET composite format syntax
Description
Fixes legacy .NET format syntax
{CommitsSinceVersionSource:0000;;''}that was outputting literal text instead of formatted values.LegacyCompositeFormatterfor semicolon-separated format sectionsValueFormatterandRegexPatternsto support semicolons??syntaxBefore:
6.13.54-gv6-CommitsSinceVersionSource-0000-----After:
6.13.54-gv60002Related Issue
Fixes #4654
Motivation and Context
Legacy .NET semicolon format syntax stopped working, breaking existing GitVersion configurations.
How Has This Been Tested?
Added comprehensive test suites:
BackwardCompatibilityTests.cs- 8 tests covering legacy format scenariosIssue4654Tests.cs- 4 tests for the specific issueAll 12 new tests pass. No regression in existing tests.
Screenshots (if appropriate):
N/A
Checklist: