Convert AgeInMonths to a source generated value #117

rabuckley · 2024-06-17T09:20:08Z

Closes #2

src/DSE.Open.Values/AgeInMonths.cs

rabuckley · 2024-06-17T09:24:11Z

src/DSE.Open.Values/AgeInMonths.cs

    /// <summary>
-    /// The number of complete years represented by the value.
+    /// The total number of months represented by the value.
    /// </summary>
-    public int Years => TotalMonths / 12;
+    public int TotalMonths { get; }

    /// <summary>
-    /// The number of months represented by the value in addition to the <see cref="Years"/>.
+    /// The total number of years and months represented by the value.
    /// </summary>
-    public int Months => TotalMonths % 12;


From an API use perspective, it felt like Months should be returning TotalMonths, so to make it clearer that it is the remainder of dividing by Years I went with a single method returning a tuple.

I think I was aiming to follow the pattern here: https://learn.microsoft.com/en-us/dotnet/api/system.timespan?view=net-8.0#properties and https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs,170

For me, it makes less sense here. Years is not a part of months in the same way that seconds is a part of a TimeSpan.

That said, I don't much mind.

src/DSE.Open.Values/AgeInMonths.cs

rabuckley · 2024-06-17T09:25:34Z

src/DSE.Open.Values/AgeInMonths.cs

+        if (!int.TryParse(s[ranges[0]], NumberStyles.None, NumberFormatInfo.InvariantInfo, out var years) ||
+            !int.TryParse(s[ranges[1]], NumberStyles.None, NumberFormatInfo.InvariantInfo, out var months))


I've gone with the strictest parsing available. Do we want to allow whitespace?

Yes - I think some leniency can be helpful

src/DSE.Open.Values/DateMonthOnly.cs

codecov · 2024-06-17T09:42:02Z

Codecov Report

Attention: Patch coverage is 28.30189% with 190 lines in your changes missing coverage. Please review.

Project coverage is 48.4%. Comparing base (fc5d8bf) to head (69bfff3).
Report is 8 commits behind head on main.

Files	Patch %	Lines
....Values.Generators.ValueTypesGenerator/Months.g.cs	21.9%	62 Missing and 2 partials ⚠️
src/DSE.Open.Values/AgeInMonths.cs	43.0%	45 Missing and 8 partials ⚠️
...es.Generators.ValueTypesGenerator/AgeInMonths.g.cs	16.1%	48 Missing and 4 partials ⚠️
src/DSE.Open.Values/Months.cs	30.0%	7 Missing ⚠️
...n.Values.Generators/ValueTypesGenerator.Emitter.cs	0.0%	4 Missing ⚠️
...nerators.ValueTypesGenerator/AlphaNumericCode.g.cs	0.0%	2 Missing ⚠️
...Values.Generators.ValueTypesGenerator/Percent.g.cs	0.0%	2 Missing ⚠️
...n.Values.Generators.ValueTypesGenerator/Ratio.g.cs	0.0%	2 Missing ⚠️
...rs/Extensions/MethodDeclarationSyntaxExtensions.cs	0.0%	1 Missing ⚠️
...lues.Generators.ValueTypesGenerator/AlphaCode.g.cs	50.0%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #117     +/-   ##
=======================================
- Coverage   48.6%   48.4%   -0.2%     
=======================================
  Files        605     606      +1     
  Lines      32491   32658    +167     
  Branches    2717    2735     +18     
=======================================
+ Hits       15800   15830     +30     
- Misses     15650   15772    +122     
- Partials    1041    1056     +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rabuckley · 2024-06-17T09:48:14Z

I didn't do IDivisibleValue because dividing months by months doesn't make sense to me. We would want to divide by int but the interface doesn't allow for that, and it is not built into the source generator.

frankbuckley

Some things to think about.

src/DSE.Open.Values/AgeInMonths.cs

frankbuckley · 2024-06-17T12:07:59Z

src/DSE.Open.Values/AgeInMonths.cs

    /// <summary>
-    /// The number of complete years represented by the value.
+    /// The total number of months represented by the value.
    /// </summary>
-    public int Years => TotalMonths / 12;
+    public int TotalMonths { get; }

    /// <summary>
-    /// The number of months represented by the value in addition to the <see cref="Years"/>.
+    /// The total number of years and months represented by the value.
    /// </summary>
-    public int Months => TotalMonths % 12;


I think I was aiming to follow the pattern here: https://learn.microsoft.com/en-us/dotnet/api/system.timespan?view=net-8.0#properties and https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs,170

src/DSE.Open.Values/AgeInMonths.cs

frankbuckley · 2024-06-17T12:10:26Z

src/DSE.Open.Values/AgeInMonths.cs

+        if (!int.TryParse(s[ranges[0]], NumberStyles.None, NumberFormatInfo.InvariantInfo, out var years) ||
+            !int.TryParse(s[ranges[1]], NumberStyles.None, NumberFormatInfo.InvariantInfo, out var months))


Yes - I think some leniency can be helpful

src/DSE.Open.Values/DateMonthOnly.cs

rabuckley · 2024-06-17T14:27:50Z

We could decide AgeInMonths is just that a record of an age at a point in time (therefore constrained to sensible, non-negative values). And then we could have a Months unit of measure type that implements IInterval<TSelf, T>.

If we did this, then the difference between two AgeInMonths value could return a Months value.

What is the distinction between AgeInMonths and Months? There are no interesting methods here that would differ. Perhaps we turn this into Months and handle ages using Months too?

frankbuckley · 2024-06-17T16:03:59Z

What is the distinction between AgeInMonths and Months? There are no interesting methods here that would differ. Perhaps we turn this into Months and handle ages using Months too?

The validation that the value is not an obviously incorrect age.

frankbuckley · 2024-06-17T16:05:06Z

An analogy:

AlphaCode is to AsciiString is to byte array what AgeInMonths is to Months is to int.

rabuckley added 2 commits June 17, 2024 10:19

Fix detection of IParsable.ToString in source generator

1d2e4cf

Convert AgeInMonths to a source generated value

666396f

Closes #2

rabuckley added the area-values label Jun 17, 2024

rabuckley requested a review from frankbuckley June 17, 2024 09:20

rabuckley self-assigned this Jun 17, 2024

rabuckley commented Jun 17, 2024

View reviewed changes

Return int from DateMoneyOnly subtraction

82a5877

rabuckley commented Jun 17, 2024

View reviewed changes

src/DSE.Open.Values/DateMonthOnly.cs Show resolved Hide resolved

frankbuckley requested changes Jun 17, 2024

View reviewed changes

rabuckley added 2 commits July 6, 2024 14:30

Update source generator with correct name in IUtf8Parsable methods

8797acf

Split out Months and AgeInMonths

69bfff3

frankbuckley approved these changes Jul 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert AgeInMonths to a source generated value #117

Convert AgeInMonths to a source generated value #117

rabuckley commented Jun 17, 2024

rabuckley Jun 17, 2024

frankbuckley Jun 17, 2024

rabuckley Jun 17, 2024

rabuckley Jun 17, 2024

frankbuckley Jun 17, 2024

codecov bot commented Jun 17, 2024 •

edited

Loading

rabuckley commented Jun 17, 2024

frankbuckley left a comment

frankbuckley Jun 17, 2024

frankbuckley Jun 17, 2024

rabuckley commented Jun 17, 2024

frankbuckley commented Jun 17, 2024

frankbuckley commented Jun 17, 2024

		if (!int.TryParse(s[ranges[0]], NumberStyles.None, NumberFormatInfo.InvariantInfo, out var years) \|\|
		!int.TryParse(s[ranges[1]], NumberStyles.None, NumberFormatInfo.InvariantInfo, out var months))

Convert AgeInMonths to a source generated value #117

Are you sure you want to change the base?

Convert AgeInMonths to a source generated value #117

Conversation

rabuckley commented Jun 17, 2024

rabuckley Jun 17, 2024

Choose a reason for hiding this comment

frankbuckley Jun 17, 2024

Choose a reason for hiding this comment

rabuckley Jun 17, 2024

Choose a reason for hiding this comment

rabuckley Jun 17, 2024

Choose a reason for hiding this comment

frankbuckley Jun 17, 2024

Choose a reason for hiding this comment

codecov bot commented Jun 17, 2024 • edited Loading

Codecov Report

rabuckley commented Jun 17, 2024

frankbuckley left a comment

Choose a reason for hiding this comment

frankbuckley Jun 17, 2024

Choose a reason for hiding this comment

frankbuckley Jun 17, 2024

Choose a reason for hiding this comment

rabuckley commented Jun 17, 2024

frankbuckley commented Jun 17, 2024

frankbuckley commented Jun 17, 2024

codecov bot commented Jun 17, 2024 •

edited

Loading