feat: CustomPeriod and additional methods to handle any flavour of period #58
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant SD as StructDefinition
participant SG as StructGenerator
participant ST as ScribanTemplate Engine
participant GS as Generated Source File
participant DR as Diagnostic Reporter
SD->>SG: Pass structDefinition (with Period property)
alt Period is not null
SG->>SG: Execute try block
SG->>ST: Load and parse embedded template
ST-->>SG: Render period methods code
SG->>GS: Add generated source file ("<StructName>.Period.Methods.g.cs")
else Exception occurs
SG->>DR: Report diagnostic error with code "CHRONO006"
end
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
src/Chrononuensis.Core/DateOnlyExtensions.cs (2)
9-9: Consider making the class public for reusability.The utility methods in this class could be useful outside the assembly. Consider making it public unless there's a specific reason to keep it internal.
-internal static class DateOnlyExtensions +public static class DateOnlyExtensions
11-18: Consider using LINQ for a more concise implementation.The current implementation is correct but could be more concise using LINQ's
MaxandMinmethods.- public static DateOnly Max(DateOnly dateOnly, params DateOnly[] others) - { - var max = dateOnly; - foreach (var other in others) - if (other > max) - max = other; - return max; - } + public static DateOnly Max(DateOnly dateOnly, params DateOnly[] others) + => others.Append(dateOnly).Max(); - public static DateOnly Min(DateOnly dateOnly, params DateOnly[] others) - { - var min = dateOnly; - foreach (var other in others) - if (other < min) - min = other; - return min; - } + public static DateOnly Min(DateOnly dateOnly, params DateOnly[] others) + => others.Append(dateOnly).Min();Also applies to: 20-27
src/Chrononuensis.Core.Testing/CustomPeriodTests.cs (1)
1-44: Add tests for new IPeriod methods.The test class is missing coverage for the new methods added to IPeriod interface:
- Contains
- Overlaps
- Meets
- Precedes
- Succeeds
- Intersect
- Span
- Gap
Would you like me to generate test cases for these methods?
src/Chrononuensis.Core/IPeriod.cs (2)
77-80: Enhance Gap method documentation.The documentation should clarify the return value when periods overlap.
/// <summary> /// Determines the gap (number of days) between two non-overlapping periods. + /// Returns a negative value if the periods overlap. /// </summary>
72-75: Add example to Span method documentation.The documentation would benefit from an example to illustrate the concept of "smallest enclosing period".
/// <summary> /// Returns the span of two periods, merging them into the smallest enclosing period. + /// For example, if period1 is [2024-01-01, 2024-01-10] and period2 is [2024-01-05, 2024-01-15], + /// the span would be [2024-01-01, 2024-01-15]. /// </summary>src/Chrononuensis.Core/CustomPeriod.cs (4)
14-19: Enhance error message clarity.While the validation logic is correct, consider making the error message more user-friendly.
- throw new ArgumentException($"The start date ({firstDate}) cannot be later than the end date ({lastDate}).", nameof(firstDate)); + throw new ArgumentException($"Invalid period: Start date {firstDate:d} must be on or before end date {lastDate:d}.", nameof(firstDate));
76-79: Consider simplifying the Gap calculation.The current implementation can be made more readable by extracting the gap calculation logic.
- public int Gap(IPeriod other) => - Overlaps(other) || Meets(other) ? 0 : - other.FirstDate > LastDate ? other.FirstDate.DayNumber - LastDate.DayNumber - 1 : - FirstDate.DayNumber - other.LastDate.DayNumber - 1; + public int Gap(IPeriod other) + { + if (Overlaps(other) || Meets(other)) + return 0; + + var (earlierPeriod, laterPeriod) = other.FirstDate > LastDate + ? (this, other) + : (other, this); + + return laterPeriod.FirstDate.DayNumber - earlierPeriod.LastDate.DayNumber - 1; + }
81-81: Consider using a standard date format string.The current format could be made more standard and culture-invariant.
- public override string ToString() => $"Custom period: {FirstDate} - {LastDate}"; + public override string ToString() => $"{FirstDate:yyyy-MM-dd} to {LastDate:yyyy-MM-dd}";
91-100: Consider reducing duplication in operator implementations.The <= and >= operators have duplicated logic across multiple overloads.
Consider extracting the common logic into private methods:
+ private static bool IsLessThanOrEqual(IPeriod left, IPeriod right) + => left.Precedes(right) || left.LastDate == right.FirstDate; + + private static bool IsGreaterThanOrEqual(IPeriod left, IPeriod right) + => left.Succeeds(right) || left.LastDate == right.FirstDate; + - public static bool operator <=(CustomPeriod left, CustomPeriod right) => left.Precedes(right) || left.LastDate == right.FirstDate; - public static bool operator >=(CustomPeriod left, CustomPeriod right) => left.Succeeds(right) || left.LastDate == right.FirstDate; - public static bool operator <=(CustomPeriod left, IPeriod right) => left.Precedes(right) || left.LastDate == right.FirstDate; - public static bool operator >=(CustomPeriod left, IPeriod right) => left.Succeeds(right) || left.LastDate == right.FirstDate; - public static bool operator <=(IPeriod left, CustomPeriod right) => left.Precedes(right) || left.LastDate == right.FirstDate; - public static bool operator >=(IPeriod left, CustomPeriod right) => left.Succeeds(right) || left.LastDate == right.FirstDate; + public static bool operator <=(CustomPeriod left, CustomPeriod right) => IsLessThanOrEqual(left, right); + public static bool operator >=(CustomPeriod left, CustomPeriod right) => IsGreaterThanOrEqual(left, right); + public static bool operator <=(CustomPeriod left, IPeriod right) => IsLessThanOrEqual(left, right); + public static bool operator >=(CustomPeriod left, IPeriod right) => IsGreaterThanOrEqual(left, right); + public static bool operator <=(IPeriod left, CustomPeriod right) => IsLessThanOrEqual(left, right); + public static bool operator >=(IPeriod left, CustomPeriod right) => IsGreaterThanOrEqual(left, right);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Chrononuensis.Core.Testing/CustomPeriodTests.cs(1 hunks)src/Chrononuensis.Core/CustomPeriod.cs(1 hunks)src/Chrononuensis.Core/DateOnlyExtensions.cs(1 hunks)src/Chrononuensis.Core/IPeriod.cs(2 hunks)src/Chrononuensis.SourceGenerator/Templates/Period.scriban-cs(2 hunks)
🔇 Additional comments (3)
src/Chrononuensis.SourceGenerator/Templates/Period.scriban-cs (1)
1-1: LGTM! The changes enhance type safety and equality comparison.The addition of nullable reference types and the
Equalsmethod implementation follow best practices:
- Proper null check using pattern matching
- Correct property comparison for equality
Also applies to: 36-37
src/Chrononuensis.Core/CustomPeriod.cs (2)
8-12: Well-designed immutable struct implementation!Good use of
readonly structwithinitproperties to ensure immutability.
27-54: Excellent implementation of period comparison methods!The methods are well-documented, efficient, and use clear boolean expressions for comparisons.
| public void LessThanOrEqual_CustomPeriod_False() | ||
| => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)) <= new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)), Is.False); |
There was a problem hiding this comment.
Fix incorrect test assertion.
The test LessThanOrEqual_CustomPeriod_False compares equal periods but expects false. For equal periods, <= should return true.
- public void LessThanOrEqual_CustomPeriod_False()
- => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)) <= new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)), Is.False);
+ public void LessThanOrEqual_CustomPeriod_True()
+ => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)) <= new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)), Is.True);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void LessThanOrEqual_CustomPeriod_False() | |
| => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)) <= new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)), Is.False); | |
| public void LessThanOrEqual_CustomPeriod_True() | |
| => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)) <= new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)), Is.True); |
| public void NotEqual_CustomPeriod_True() | ||
| => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)) != new YearMonth(2024, 1), Is.False); |
There was a problem hiding this comment.
Fix misleading test name.
The test name NotEqual_CustomPeriod_True suggests expecting true but asserts false, making it confusing.
- public void NotEqual_CustomPeriod_True()
- => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)) != new YearMonth(2024, 1), Is.False);
+ public void NotEqual_CustomPeriod_False()
+ => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)) != new YearMonth(2024, 1), Is.False);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void NotEqual_CustomPeriod_True() | |
| => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)) != new YearMonth(2024, 1), Is.False); | |
| public void NotEqual_CustomPeriod_False() | |
| => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)) != new YearMonth(2024, 1), Is.False); |
| (FirstDate, LastDate) = (firstDate, lastDate); | ||
| } | ||
|
|
||
| public int Days => FirstDate.DayNumber - LastDate.DayNumber + 1; |
There was a problem hiding this comment.
Fix incorrect Days calculation.
The current calculation will give negative results as it subtracts LastDate from FirstDate. The order should be reversed.
- public int Days => FirstDate.DayNumber - LastDate.DayNumber + 1;
+ public int Days => LastDate.DayNumber - FirstDate.DayNumber + 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public int Days => FirstDate.DayNumber - LastDate.DayNumber + 1; | |
| public int Days => LastDate.DayNumber - FirstDate.DayNumber + 1; |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Chrononuensis.Core.Testing/CustomPeriodTests.cs (1)
53-55:⚠️ Potential issueFix incorrect test assertion and name.
The test name suggests expecting false but asserts true. This is inconsistent and confusing.
- public void LessThanOrEqual_CustomPeriod_False() - => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)) - <= new CustomPeriod(new DateOnly(2025, 1, 10), new DateOnly(2025, 1, 20)), Is.True); + public void LessThanOrEqual_CustomPeriod_True() + => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)) + <= new CustomPeriod(new DateOnly(2025, 1, 10), new DateOnly(2025, 1, 20)), Is.True);
🧹 Nitpick comments (5)
src/Chrononuensis.Core.Testing/CustomPeriodTests.cs (5)
11-14: Improve constructor test by asserting state.Instead of using
DoesNotThrow, consider asserting the actual state of the created object to ensure it's initialized correctly.- public void Ctor_ValidValues_Expected() - => Assert.DoesNotThrow(() => new CustomPeriod(new DateOnly(2025,1,1), new DateOnly(2025, 1, 10))); + public void Ctor_ValidValues_Expected() + { + var period = new CustomPeriod(new DateOnly(2025,1,1), new DateOnly(2025, 1, 10)); + Assert.Multiple(() => + { + Assert.That(period.FirstDate, Is.EqualTo(new DateOnly(2025,1,1))); + Assert.That(period.LastDate, Is.EqualTo(new DateOnly(2025, 1, 10))); + }); + }
15-19: Verify exception message in constructor test.The test captures the exception but doesn't verify its message. Consider asserting the exception message to ensure it provides meaningful feedback.
public void Ctor_InvalidValues_Throws() { - var ex = Assert.Throws<ArgumentException>(() => new CustomPeriod(new DateOnly(2025, 1, 10), new DateOnly(2025, 1, 1))); + var ex = Assert.Throws<ArgumentException>(() => + new CustomPeriod(new DateOnly(2025, 1, 10), new DateOnly(2025, 1, 1))); + Assert.That(ex.Message, Does.Contain("LastDate must be greater than or equal to FirstDate")); }
26-27: Fix typo in test method name.The test method name has a typo: "FirsDate" should be "FirstDate".
- public void FirsDate_ValidValues_Expected() + public void FirstDate_ValidValues_Expected()
21-24: Add edge cases for Days property test.Consider adding test cases for edge cases like single-day periods and month boundaries.
+ [TestCase("2025-01-01", "2025-01-01", 1, Description = "Single day")] + [TestCase("2025-01-31", "2025-02-01", 2, Description = "Month boundary")] + [TestCase("2024-02-28", "2024-03-01", 3, Description = "Month boundary in leap year")] + public void Days_ValidValues_Expected(string start, string end, int expectedDays) + { + var period = new CustomPeriod(DateOnly.Parse(start), DateOnly.Parse(end)); + Assert.That(period.Days, Is.EqualTo(expectedDays)); + } - public void Days_ValidValues_Expected() - => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)).Days, Is.EqualTo(10));
77-93: Add missing equality test cases.Consider adding tests for null comparison and different type comparison to ensure complete coverage of equality scenarios.
+ [Test] + public void Equal_Null_False() + { + var period = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)); + Assert.That(period.Equals(null), Is.False); + } + + [Test] + public void Equal_DifferentType_False() + { + var period = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)); + Assert.That(period.Equals("not a period"), Is.False); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Chrononuensis.Core.Testing/CustomPeriodTests.cs(1 hunks)src/Chrononuensis.Core/CustomPeriod.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Chrononuensis.Core/CustomPeriod.cs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/Chrononuensis.SourceGenerator/StructGenerator.cs (1)
263-282: Consider enriching the template model with additional context.The model passed to the template is minimal compared to other generator methods. Consider passing additional context that might be useful for generating period methods, such as:
partsfor type informationperiodconfigurationby_year,first,lastvalues (as used inGeneratePeriod)Apply this diff to enrich the model:
var output = scribanTemplate.Render(new { struct_name = structDefinition.Name, + parts = structDefinition.Parts?.Select(p => new + { + name = p.Name, + type = p.Type + }).ToList(), + period = new + { + by_year = structDefinition.Period.ByYear, + first = structDefinition.Period.First, + last = structDefinition.Period.Last, + year = new + { + value = structDefinition.Period.Year, + duration = structDefinition.Period.YearDuration + } + } });src/Chrononuensis.SourceGenerator/Templates/Period.Methods.scriban-cs (2)
24-25: Optimize theMeetsmethod implementation.The current implementation checks both boundaries. Since periods can only meet at one boundary, we can optimize by first determining which period comes first.
- public bool Meets(IPeriod other) => - LastDate.AddDays(1) == other.FirstDate || other.LastDate.AddDays(1) == FirstDate; + public bool Meets(IPeriod other) => + FirstDate <= other.FirstDate + ? LastDate.AddDays(1) == other.FirstDate + : other.LastDate.AddDays(1) == FirstDate;
58-61: Simplify theGapmethod implementation.The current implementation has multiple conditional branches. We can simplify it by using
Math.Absto handle both cases.- public int Gap(IPeriod other) => - Overlaps(other) || Meets(other) ? 0 : - other.FirstDate > LastDate ? other.FirstDate.DayNumber - LastDate.DayNumber - 1 : - FirstDate.DayNumber - other.LastDate.DayNumber - 1; + public int Gap(IPeriod other) => + Overlaps(other) || Meets(other) ? 0 : + Math.Abs( + FirstDate <= other.FirstDate + ? other.FirstDate.DayNumber - LastDate.DayNumber - 1 + : FirstDate.DayNumber - other.LastDate.DayNumber - 1);src/Chrononuensis.Core.Testing/PeriodMethodsTests.cs (3)
202-204: Fix incorrect XML comment.The XML comment for
SpanTestCasesincorrectly states it's for "Intersect".- /// <summary> - /// Manually defined test cases for Intersect. - /// </summary> + /// <summary> + /// Manually defined test cases for Span. + /// </summary>
233-235: Fix incorrect XML comment.The XML comment for
GapTestCasesincorrectly states it's for "Intersect".- /// <summary> - /// Manually defined test cases for Intersect. - /// </summary> + /// <summary> + /// Manually defined test cases for Gap. + /// </summary>
27-37: Add descriptive test case names.The test cases would be more maintainable with descriptive names that explain the scenario being tested.
Example for
ContainsTestCases:- yield return new TestCaseData(century, decade).Returns(false); + yield return new TestCaseData(century, decade) + .SetName("Century does not contain Decade outside its range") + .Returns(false);Also applies to: 58-68, 89-99, 121-131, 153-165, 185-195, 216-226, 247-257
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Chrononuensis.Core.Testing/CustomPeriodTests.cs(1 hunks)src/Chrononuensis.Core.Testing/PeriodMethodsTests.cs(1 hunks)src/Chrononuensis.SourceGenerator/Chrononuensis.SourceGenerator.csproj(1 hunks)src/Chrononuensis.SourceGenerator/StructGenerator.cs(2 hunks)src/Chrononuensis.SourceGenerator/Templates/Period.Methods.scriban-cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Chrononuensis.Core.Testing/CustomPeriodTests.cs
🔇 Additional comments (3)
src/Chrononuensis.SourceGenerator/StructGenerator.cs (1)
91-108: LGTM! Error handling follows consistent pattern.The try-catch block follows the established pattern in the file, with proper error reporting and a unique error code.
src/Chrononuensis.SourceGenerator/Chrononuensis.SourceGenerator.csproj (1)
34-34: LGTM! Resource inclusion follows consistent pattern.The new embedded resource follows the established pattern and is correctly placed alongside other period-related templates.
src/Chrononuensis.Core.Testing/PeriodMethodsTests.cs (1)
1-263: LGTM! Comprehensive test coverage.The test suite provides excellent coverage of the period manipulation methods, including edge cases and various period combinations.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #58 +/- ##
==========================================
- Coverage 91.61% 85.75% -5.87%
==========================================
Files 23 24 +1
Lines 334 400 +66
Branches 45 60 +15
==========================================
+ Hits 306 343 +37
- Misses 13 33 +20
- Partials 15 24 +9 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Chrononuensis.Core/CustomPeriod.cs (2)
10-13: Add XML documentation for public members.Consider adding XML documentation for the public properties to improve IntelliSense support and API documentation.
+ /// <summary> + /// Gets the first date of the period. + /// </summary> public DateOnly FirstDate { get; init; } + /// <summary> + /// Gets the last date of the period. + /// </summary> public DateOnly LastDate { get; init; } + /// <summary> + /// Gets the total number of days in the period, inclusive of both first and last dates. + /// </summary> public int Days => LastDate.DayNumber - FirstDate.DayNumber + 1; + /// <summary> + /// Gets the lower bound of the period as a DateTime at midnight. + /// </summary> public DateTime LowerBound => FirstDate.ToDateTime(TimeOnly.MinValue); + /// <summary> + /// Gets the upper bound of the period as a DateTime at midnight of the next day. + /// </summary> public DateTime UpperBound => LastDate.AddDays(1).ToDateTime(TimeOnly.MinValue);Also applies to: 21-25
73-86: Consider handling negative gaps.The Gap method correctly handles overlapping periods and meets by returning 0. However, consider throwing an ArgumentException for negative gaps to catch potential bugs in the calling code.
public int Gap(IPeriod other) { if (Overlaps(other) || Meets(other)) return 0; (var earlierPeriod, var laterPeriod) = other.FirstDate > LastDate ? ((IPeriod)this, other) : (other, this); - return laterPeriod.FirstDate.DayNumber - earlierPeriod.LastDate.DayNumber - 1; + var gap = laterPeriod.FirstDate.DayNumber - earlierPeriod.LastDate.DayNumber - 1; + if (gap < 0) + throw new ArgumentException($"Invalid gap: {gap} days. This indicates a bug as periods should not overlap.", nameof(other)); + return gap; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Chrononuensis.Core.Testing/CustomPeriodTests.cs(1 hunks)src/Chrononuensis.Core/CustomPeriod.cs(1 hunks)src/Chrononuensis.SourceGenerator/Templates/Period.Methods.scriban-cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Chrononuensis.Core.Testing/CustomPeriodTests.cs
- src/Chrononuensis.SourceGenerator/Templates/Period.Methods.scriban-cs
🔇 Additional comments (4)
src/Chrononuensis.Core/CustomPeriod.cs (4)
8-13: LGTM! Well-designed immutable struct.The struct is correctly marked as readonly with init-only properties, ensuring thread-safety and preventing mutation after construction.
27-54: LGTM! Well-implemented period comparison methods.The period comparison methods are logically correct and well-documented:
- Contains checks full containment
- Overlaps checks any intersection
- Meets checks adjacency
- Precedes/Succeeds check strict ordering
55-72: LGTM! Well-implemented Intersect and Span methods.The methods correctly handle period intersections and merging:
- Intersect returns null for non-overlapping periods
- Span creates the smallest enclosing period
88-109: LGTM! Well-implemented equality methods.The equality methods follow best practices:
- Null checks
- Type checking
- Consistent GetHashCode
| private static bool IsLessThanOrEqual(IPeriod left, IPeriod right) | ||
| => left.FirstDate <= right.FirstDate && left.LastDate <= right.FirstDate; | ||
|
|
||
| private static bool IsGreaterThanOrEqual(IPeriod left, IPeriod right) | ||
| => left.FirstDate >= right.FirstDate && left.LastDate >= right.FirstDate; | ||
|
|
There was a problem hiding this comment.
Fix incorrect comparison logic in IsLessThanOrEqual and IsGreaterThanOrEqual.
The comparison methods have bugs that could lead to incorrect results:
- IsLessThanOrEqual compares LastDate with FirstDate instead of right.LastDate
- IsGreaterThanOrEqual has the same issue
private static bool IsLessThanOrEqual(IPeriod left, IPeriod right)
- => left.FirstDate <= right.FirstDate && left.LastDate <= right.FirstDate;
+ => left.FirstDate <= right.FirstDate && left.LastDate <= right.LastDate;
private static bool IsGreaterThanOrEqual(IPeriod left, IPeriod right)
- => left.FirstDate >= right.FirstDate && left.LastDate >= right.FirstDate;
+ => left.FirstDate >= right.FirstDate && left.LastDate >= right.LastDate;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static bool IsLessThanOrEqual(IPeriod left, IPeriod right) | |
| => left.FirstDate <= right.FirstDate && left.LastDate <= right.FirstDate; | |
| private static bool IsGreaterThanOrEqual(IPeriod left, IPeriod right) | |
| => left.FirstDate >= right.FirstDate && left.LastDate >= right.FirstDate; | |
| private static bool IsLessThanOrEqual(IPeriod left, IPeriod right) | |
| => left.FirstDate <= right.FirstDate && left.LastDate <= right.LastDate; | |
| private static bool IsGreaterThanOrEqual(IPeriod left, IPeriod right) | |
| => left.FirstDate >= right.FirstDate && left.LastDate >= right.LastDate; |
| FirstDate <= other.LastDate && LastDate >= other.FirstDate; | ||
|
|
||
| /// <summary> | ||
| /// Determines if this period meets another period (i.e., is adjacent without overlap). | ||
| /// </summary> | ||
| public bool Meets(IPeriod other) => | ||
| LastDate.AddDays(1) == other.FirstDate || other.LastDate.AddDays(1) == FirstDate; | ||
|
|
||
| /// <summary> | ||
| /// Determines if this period is strictly before another. | ||
| /// </summary> | ||
| public bool Precedes(IPeriod other) => LastDate < other.FirstDate; | ||
|
|
||
| /// <summary> | ||
| /// Determines if this period is strictly after another. | ||
| /// </summary> | ||
| public bool Succeeds(IPeriod other) => FirstDate > other.LastDate; | ||
|
|
||
| /// <summary> | ||
| /// Returns the intersection of two periods if they overlap. | ||
| /// </summary> | ||
| public IPeriod? Intersect(IPeriod other) => | ||
| Overlaps(other) | ||
| ? new CustomPeriod( | ||
| new[] { FirstDate, other.FirstDate }.Max(), | ||
| new[] { LastDate, other.LastDate }.Min()) | ||
| : null; | ||
|
|
||
| /// <summary> | ||
| /// Returns the span of two periods, merging them into the smallest enclosing period. | ||
| /// </summary> | ||
| public IPeriod Span(IPeriod other) => | ||
| new CustomPeriod( | ||
| new[] { FirstDate, other.FirstDate }.Min(), | ||
| new[] { LastDate, other.LastDate }.Max()); | ||
|
|
||
| /// <summary> | ||
| /// Determines the gap (number of days) between two non-overlapping periods. | ||
| /// </summary> | ||
| public int Gap(IPeriod other) | ||
| { | ||
| if (Overlaps(other) || Meets(other)) | ||
| return 0; | ||
|
|
||
| (var earlierPeriod, var laterPeriod) = other.FirstDate > LastDate | ||
| ? ((IPeriod)this, other) | ||
| : (other, this); | ||
|
|
||
| return laterPeriod.FirstDate.DayNumber - earlierPeriod.LastDate.DayNumber - 1; | ||
| } | ||
|
|
||
| public override string ToString() => $"Custom period: {FirstDate} - {LastDate}"; | ||
|
|
||
|
|
||
| public bool Equals(IPeriod? other) | ||
| => other switch | ||
| { | ||
| null => false, | ||
| CustomPeriod period => this == period, | ||
| _ => FirstDate == other.FirstDate && LastDate == other.LastDate | ||
| }; | ||
|
|
||
| public override bool Equals(object? obj) | ||
| => obj switch | ||
| { | ||
| null => false, | ||
| CustomPeriod period => this == period, | ||
| IPeriod period => this == period, | ||
| _ => false | ||
| }; | ||
|
|
||
| public override int GetHashCode() => HashCode.Combine(FirstDate, LastDate); | ||
|
|
||
| public static bool operator <(CustomPeriod left, CustomPeriod right) => left.Precedes(right); | ||
| public static bool operator >(CustomPeriod left, CustomPeriod right) => left.Succeeds(right); | ||
| public static bool operator <(CustomPeriod left, IPeriod right) => left.Precedes(right); | ||
| public static bool operator >(CustomPeriod left, IPeriod right) => left.Succeeds(right); | ||
| public static bool operator <(IPeriod left, CustomPeriod right) => left.Precedes(right); | ||
| public static bool operator >(IPeriod left, CustomPeriod right) => left.Succeeds(right); | ||
|
|
||
| private static bool IsLessThanOrEqual(IPeriod left, IPeriod right) | ||
| => left.FirstDate <= right.FirstDate && left.LastDate <= right.FirstDate; | ||
|
|
||
| private static bool IsGreaterThanOrEqual(IPeriod left, IPeriod right) | ||
| => left.FirstDate >= right.FirstDate && left.LastDate >= right.FirstDate; | ||
|
|
||
| public static bool operator <=(CustomPeriod left, CustomPeriod right) => IsLessThanOrEqual(left, right); | ||
| public static bool operator >=(CustomPeriod left, CustomPeriod right) => IsGreaterThanOrEqual(left, right); | ||
| public static bool operator <=(CustomPeriod left, IPeriod right) => IsLessThanOrEqual(left, right); | ||
| public static bool operator >=(CustomPeriod left, IPeriod right) => IsGreaterThanOrEqual(left, right); | ||
| public static bool operator <=(IPeriod left, CustomPeriod right) => IsLessThanOrEqual(left, right); | ||
| public static bool operator >=(IPeriod left, CustomPeriod right) => IsGreaterThanOrEqual(left, right); | ||
|
|
||
| public static bool operator ==(CustomPeriod left, CustomPeriod right) => left.FirstDate == right.FirstDate && left.LastDate == right.LastDate; | ||
| public static bool operator !=(CustomPeriod left, CustomPeriod right) => !(left == right); | ||
| public static bool operator ==(CustomPeriod left, IPeriod right) => left.FirstDate == right.FirstDate && left.LastDate == right.LastDate; | ||
| public static bool operator !=(CustomPeriod left, IPeriod right) => !(left == right); | ||
| public static bool operator ==(IPeriod left, CustomPeriod right) => left.FirstDate == right.FirstDate && left.LastDate == right.LastDate; | ||
| public static bool operator !=(IPeriod left, CustomPeriod right) => !(left == right); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Improve test coverage.
The Codecov report indicates low patch coverage (38.98%) with 33 lines missing coverage. Consider adding test cases for:
- Edge cases in comparison methods (e.g., adjacent periods, single-day periods)
- Operator overloads (especially <=, >=)
- Error conditions (e.g., invalid periods, negative gaps)
🏁 Script executed:
#!/bin/bash
# Description: Find existing test files for CustomPeriod
echo "Searching for test files..."
fd -e cs "CustomPeriod.*Test" -x cat {}Length of output: 5266
Enhance test coverage for period operations.
The existing test suite confirms correct behavior for constructors, basic properties, and a few operator overloads. However, many key period methods remain untested. In particular, please add tests that cover:
- Comparison methods: Validate edge cases for
Contains,Overlaps,Meets,Precedes, andSucceeds(e.g., adjacent periods and single-day periods). - Operator overloads: Extend coverage for all overloads, especially those that mix
CustomPeriodandIPeriod. - Additional logic: Exercise methods like
Intersect,Span, and especiallyGap(including non-overlapping periods where the gap should be correctly computed).
These additions should help improve the overall patch coverage and safeguard against potential boundary issues.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/Chrononuensis.Core.Testing/CustomPeriodTests.cs (1)
74-77:⚠️ Potential issueFix incorrect test name and assertion.
The test name suggests expecting false but asserts true, making it confusing. Additionally, the test case is testing overlapping periods which should be clarified in the name.
- public void LessThanOrEqual_CustomPeriod_False() - => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)) - <= new CustomPeriod(new DateOnly(2025, 1, 10), new DateOnly(2025, 1, 20)), Is.True); + public void LessThanOrEqual_OverlappingPeriods_True() + => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)) + <= new CustomPeriod(new DateOnly(2025, 1, 10), new DateOnly(2025, 1, 20)), Is.True);
🧹 Nitpick comments (2)
src/Chrononuensis.Core.Testing/CustomPeriodTests.cs (2)
37-44: Enhance test coverage for Days property.Consider adding more test cases to cover:
- Longer periods (e.g., full year)
- Periods crossing year boundaries
- Maximum supported period length
[TestCase("2025-01-01", "2025-01-01", 1, Description = "Single day")] [TestCase("2025-01-31", "2025-02-01", 2, Description = "Month boundary")] [TestCase("2024-02-28", "2024-03-01", 3, Description = "Month boundary in leap year")] +[TestCase("2024-01-01", "2024-12-31", 366, Description = "Full leap year")] +[TestCase("2024-12-31", "2025-01-01", 2, Description = "Year boundary")] +[TestCase("2025-12-31", "2026-01-01", 2, Description = "Non-leap year boundary")] public void Days_ValidValues_Expected(string start, string end, int expectedDays)
54-60: Verify DateTime components in bound tests.The tests for LowerBound and UpperBound should verify that the time component is set correctly (midnight for lower bound, midnight next day for upper bound).
- public void LowerBound_ValidValues_Expected() - => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)).LowerBound, Is.EqualTo(new DateTime(2025, 1, 1))); + public void LowerBound_ValidValues_Expected() + => Assert.Multiple(() => { + var bound = new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)).LowerBound; + Assert.That(bound.Date, Is.EqualTo(new DateTime(2025, 1, 1))); + Assert.That(bound.TimeOfDay, Is.EqualTo(TimeSpan.Zero)); + }); - public void UpperBound_ValidValues_Expected() - => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)).UpperBound, Is.EqualTo(new DateTime(2025, 1, 11))); + public void UpperBound_ValidValues_Expected() + => Assert.Multiple(() => { + var bound = new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)).UpperBound; + Assert.That(bound.Date, Is.EqualTo(new DateTime(2025, 1, 11))); + Assert.That(bound.TimeOfDay, Is.EqualTo(TimeSpan.Zero)); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Chrononuensis.Core.Testing/CustomPeriodTests.cs(1 hunks)src/Chrononuensis.Core/CustomPeriod.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Chrononuensis.Core/CustomPeriod.cs
|
|
||
| [Test] | ||
| public void LessThanOrEqual_CustomPeriod_True() | ||
| => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 10)) | ||
| <= new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)), Is.True); | ||
|
|
||
| [Test] | ||
| public void LessThan_CustomPeriod_False() | ||
| => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)) | ||
| < new CustomPeriod(new DateOnly(2025, 1, 10), new DateOnly(2025, 1, 20)), Is.False); | ||
|
|
||
| [Test] | ||
| public void LessThanOrEqual_CustomPeriod_False() | ||
| => Assert.That(new CustomPeriod(new DateOnly(2025, 1, 1), new DateOnly(2025, 1, 10)) | ||
| <= new CustomPeriod(new DateOnly(2025, 1, 10), new DateOnly(2025, 1, 20)), Is.True); | ||
|
|
||
| [Test] | ||
| public void Equal_CustomPeriod_True() | ||
| => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)) | ||
| == new YearMonth(2024, 1), Is.True); | ||
|
|
||
| [Test] | ||
| public void Equal_CustomPeriod_False() | ||
| => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)) | ||
| == new YearMonth(2025, 1), Is.False); | ||
|
|
||
| [Test] | ||
| public void NotEqual_CustomPeriod_False() | ||
| => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)) | ||
| != new YearMonth(2024, 1), Is.False); | ||
|
|
||
| [Test] | ||
| public void NotEqual_CustomPeriod_True() | ||
| => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)) | ||
| != new YearMonth(2025, 1), Is.True); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing comparison test cases.
The comparison operators need more test cases to cover:
- Equal periods
- Adjacent periods
- Completely disjoint periods
- Edge cases around period boundaries
Add these test cases:
[Test]
public void LessThan_AdjacentPeriods_True()
=> Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 10))
< new CustomPeriod(new DateOnly(2024, 1, 11), new DateOnly(2024, 1, 20)), Is.True);
[Test]
public void LessThan_DisjointPeriods_True()
=> Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 10))
< new CustomPeriod(new DateOnly(2024, 2, 1), new DateOnly(2024, 2, 10)), Is.True);
[Test]
public void LessThanOrEqual_EqualPeriods_True()
=> Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 10))
<= new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 10)), Is.True);| [Test] | ||
| public void Equal_Identical_True() | ||
| => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)).Equals( | ||
| new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31))), Is.True); | ||
|
|
||
| [Test] | ||
| public void Equal_SameSpan_True() | ||
| => Assert.That(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)).Equals( | ||
| new YearMonth(2024, 1)), Is.True); | ||
|
|
||
| [Test] | ||
| public void Equal_SameRef_True() | ||
| { | ||
| var period = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)); | ||
| Assert.That(period.Equals(period), Is.True); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Equal_Null_False() | ||
| { | ||
| var period = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)); | ||
| Assert.That(period.Equals(null), Is.False); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Equal_DifferentType_False() | ||
| { | ||
| var period = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31)); | ||
| Assert.That(period.Equals("not a period"), Is.False); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add GetHashCode test for equality contract.
When implementing equality, it's important to test GetHashCode to ensure equal objects have equal hash codes.
Add this test:
[Test]
public void GetHashCode_EqualPeriods_SameHashCode()
{
var period1 = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31));
var period2 = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31));
Assert.That(period1.GetHashCode(), Is.EqualTo(period2.GetHashCode()));
}| [Test] | ||
| public void Contains_Distinct_Expected() | ||
| { | ||
| var left = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 20)); | ||
| var right = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 10)); | ||
| Assert.That(left.Contains(right), Is.True); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Overlaps_Distinct_Expected() | ||
| { | ||
| var left = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 20)); | ||
| var right = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 2, 10)); | ||
| Assert.That(left.Overlaps(right), Is.True); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Meets_Distinct_Expected() | ||
| { | ||
| var left = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 20)); | ||
| var right = new CustomPeriod(new DateOnly(2024, 1, 21), new DateOnly(2024, 2, 10)); | ||
| Assert.That(left.Meets(right), Is.True); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Succeeds_Distinct_Expected() | ||
| { | ||
| var left = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 20)); | ||
| var right = new CustomPeriod(new DateOnly(2024, 1, 25), new DateOnly(2024, 2, 10)); | ||
| Assert.That(left.Succeeds(right), Is.False); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Precedes_Distinct_Expected() | ||
| { | ||
| var left = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 20)); | ||
| var right = new CustomPeriod(new DateOnly(2024, 1, 25), new DateOnly(2024, 2, 10)); | ||
| Assert.That(left.Precedes(right), Is.True); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Intersect_Distinct_Expected() | ||
| { | ||
| var left = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 20)); | ||
| var right = new CustomPeriod(new DateOnly(2024, 1, 15), new DateOnly(2024, 2, 10)); | ||
| Assert.That(left.Intersect(right), Is.EqualTo(new CustomPeriod(new DateOnly(2024, 1, 15), new DateOnly(2024, 1, 20)))); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Span_Distinct_Expected() | ||
| { | ||
| var left = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 20)); | ||
| var right = new CustomPeriod(new DateOnly(2024, 1, 25), new DateOnly(2024, 2, 10)); | ||
| Assert.That(left.Span(right), Is.EqualTo(new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 2, 10)))); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Gap_Distinct_Expected() | ||
| { | ||
| var left = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 20)); | ||
| var right = new CustomPeriod(new DateOnly(2024, 1, 25), new DateOnly(2024, 2, 10)); | ||
| Assert.That(left.Gap(right), Is.EqualTo(4)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance period manipulation test coverage.
Each period operation needs more test cases to cover:
- Edge cases (e.g., adjacent periods, single-day periods)
- Negative cases (non-overlapping, non-meeting periods)
- Invalid inputs (null periods)
Example additional test cases:
[Test]
public void Contains_SingleDay_Expected()
{
var container = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31));
var contained = new CustomPeriod(new DateOnly(2024, 1, 15), new DateOnly(2024, 1, 15));
Assert.That(container.Contains(contained), Is.True);
}
[Test]
public void Overlaps_NoOverlap_Expected()
{
var left = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 10));
var right = new CustomPeriod(new DateOnly(2024, 1, 20), new DateOnly(2024, 1, 31));
Assert.That(left.Overlaps(right), Is.False);
}
[Test]
public void Intersect_NullPeriod_ThrowsArgumentNullException()
{
var period = new CustomPeriod(new DateOnly(2024, 1, 1), new DateOnly(2024, 1, 31));
Assert.Throws<ArgumentNullException>(() => period.Intersect(null));
}
Seddryck
changed the title
Summary by CodeRabbit